From 3b6846698aac81211d353c4f8c9f9eddbbeed284 Mon Sep 17 00:00:00 2001 From: techmahedy Date: Mon, 2 Nov 2026 15:48:44 +0600 Subject: [PATCH 1/6] feat: Lazy, Store-Based Sessions --- src/Phaseolies/Application.php | 34 ++ .../Console/Commands/ClearSessionCommand.php | 17 +- .../Console/Commands/SessionGcCommand.php | 39 ++ src/Phaseolies/Error/WebErrorRenderer.php | 4 +- src/Phaseolies/Helpers/helpers.php | 2 +- src/Phaseolies/Http/Request.php | 5 + src/Phaseolies/Http/Response.php | 1 + src/Phaseolies/Launchers/SessionLauncher.php | 4 +- .../Middleware/CsrfTokenMiddleware.php | 29 +- .../Middleware/StatelessSession.php | 30 ++ src/Phaseolies/Session/ConfigSession.php | 22 - .../Contracts/AbstractSessionHandler.php | 62 --- .../Session/Contracts/CommittableSession.php | 46 ++ .../Contracts/SessionHandlerInterface.php | 41 -- .../Contracts/SessionStoreInterface.php | 69 +++ .../Session/Exceptions/SessionException.php | 8 + .../Session/Handlers/CookieSessionHandler.php | 269 ----------- .../Session/Handlers/FileSessionHandler.php | 80 ---- .../Handlers/SessionHandlerFactory.php | 27 -- .../Session/Handlers/SessionManager.php | 65 --- src/Phaseolies/Session/SessionManager.php | 165 +++++++ src/Phaseolies/Session/Stores/ArrayStore.php | 138 ++++++ src/Phaseolies/Session/Stores/CookieStore.php | 179 ++++++++ src/Phaseolies/Session/Stores/FileStore.php | 325 ++++++++++++++ src/Phaseolies/Support/Session.php | 423 +++++++++++++++++- tests/Error/WebErrorRendererTest.php | 17 + tests/Requests/RequestTest.php | 10 +- tests/Requests/SessionTest.php | 47 +- tests/Session/CookieSessionHandlerTest.php | 203 --------- tests/Session/CookieStoreTest.php | 67 +++ tests/Session/DispatchSessionTest.php | 147 ++++++ tests/Session/FileStoreTest.php | 273 +++++++++++ tests/Session/SessionManagerTest.php | 35 ++ tests/Session/SessionTest.php | 390 ++++++++++++++++ 34 files changed, 2439 insertions(+), 834 deletions(-) create mode 100644 src/Phaseolies/Console/Commands/SessionGcCommand.php create mode 100644 src/Phaseolies/Middleware/StatelessSession.php delete mode 100644 src/Phaseolies/Session/ConfigSession.php delete mode 100644 src/Phaseolies/Session/Contracts/AbstractSessionHandler.php create mode 100644 src/Phaseolies/Session/Contracts/CommittableSession.php delete mode 100644 src/Phaseolies/Session/Contracts/SessionHandlerInterface.php create mode 100644 src/Phaseolies/Session/Contracts/SessionStoreInterface.php create mode 100644 src/Phaseolies/Session/Exceptions/SessionException.php delete mode 100644 src/Phaseolies/Session/Handlers/CookieSessionHandler.php delete mode 100644 src/Phaseolies/Session/Handlers/FileSessionHandler.php delete mode 100644 src/Phaseolies/Session/Handlers/SessionHandlerFactory.php delete mode 100644 src/Phaseolies/Session/Handlers/SessionManager.php create mode 100644 src/Phaseolies/Session/SessionManager.php create mode 100644 src/Phaseolies/Session/Stores/ArrayStore.php create mode 100644 src/Phaseolies/Session/Stores/CookieStore.php create mode 100644 src/Phaseolies/Session/Stores/FileStore.php delete mode 100644 tests/Session/CookieSessionHandlerTest.php create mode 100644 tests/Session/CookieStoreTest.php create mode 100644 tests/Session/DispatchSessionTest.php create mode 100644 tests/Session/FileStoreTest.php create mode 100644 tests/Session/SessionManagerTest.php create mode 100644 tests/Session/SessionTest.php diff --git a/src/Phaseolies/Application.php b/src/Phaseolies/Application.php index f31b5034..eab904e1 100644 --- a/src/Phaseolies/Application.php +++ b/src/Phaseolies/Application.php @@ -11,6 +11,8 @@ use Phaseolies\Http\Request; use Phaseolies\Http\Contracts\GatewayInterface; use Phaseolies\Http\Exceptions\HttpException; +use Phaseolies\Http\Exceptions\HttpResponseException; +use Phaseolies\Session\Contracts\CommittableSession; use Phaseolies\Error\ErrorHandler; use Phaseolies\DI\Container; use Phaseolies\Config\Config; @@ -1547,6 +1549,8 @@ public function dispatch($request): DispatchResult $response = $this->handle($request); + $this->commitSession($request, $response); + $response->prepare($request)->send(); $this->suspendDeferScope($deferScope); @@ -1555,6 +1559,8 @@ public function dispatch($request): DispatchResult return new DispatchResult($this, $request, $response, null, $deferScope, $context); } catch (HttpException $e) { try { + $this->commitSession($request); + Response::dispatchHttpException($e); } catch (\Throwable $renderFailure) { $this->endDeferScope($deferScope, $renderFailure); @@ -1568,6 +1574,18 @@ public function dispatch($request): DispatchResult return new DispatchResult($this, $request, null, $e, $deferScope, $context); } catch (\Throwable $e) { + // Validation failures and abort() are thrown as exceptions that carry + // the response to send (usually a redirect with flashed errors and + // old input). The global error handler sends it, so the session + // must be saved onto that response here, before it is rethrown. + if ($e instanceof HttpResponseException && $e->hasResponse()) { + try { + $this->commitSession($request, $e->getResponse()); + } catch (\Throwable) { + // Never replace the exception that is already ending the request. + } + } + $this->endDeferScope($deferScope, $e); $this->endContext($context, $e); @@ -1575,6 +1593,22 @@ public function dispatch($request): DispatchResult } } + /** + * Persist the session of the request + * + * @param Request $request + * @param Response|null $response + * @return void + */ + protected function commitSession(Request $request, ?Response $response = null): void + { + $session = $request->session(); + + if ($session instanceof CommittableSession) { + $session->commit($response); + } + } + /** * Describe a request for its execution context. Must never throw. * diff --git a/src/Phaseolies/Console/Commands/ClearSessionCommand.php b/src/Phaseolies/Console/Commands/ClearSessionCommand.php index a18d6f82..e3acbea2 100644 --- a/src/Phaseolies/Console/Commands/ClearSessionCommand.php +++ b/src/Phaseolies/Console/Commands/ClearSessionCommand.php @@ -3,6 +3,7 @@ namespace Phaseolies\Console\Commands; use Phaseolies\Console\Schedule\Command; +use Phaseolies\Session\SessionManager; class ClearSessionCommand extends Command { @@ -14,11 +15,11 @@ class ClearSessionCommand extends Command protected $name = 'session:clear'; /** - * The name of the console command. + * The description of the console command. * * @var string */ - protected $description = 'Clear all sessions from the Applicaion.'; + protected $description = 'Clear all sessions from the Application.'; /** * Execute the console command. @@ -27,17 +28,9 @@ class ClearSessionCommand extends Command */ public function handle(): int { - return $this->withTiming(function() { - $sessionDir = base_path('storage/framework/sessions'); - - $files = glob($sessionDir . '/*'); - foreach ($files as $file) { - if (is_file($file)) { - @unlink($file); - } - } + return $this->withTiming(function () { + SessionManager::store()->flush(); - session()->flush(); return Command::SUCCESS; }, 'Application sessions have been gracefully cleared.'); } diff --git a/src/Phaseolies/Console/Commands/SessionGcCommand.php b/src/Phaseolies/Console/Commands/SessionGcCommand.php new file mode 100644 index 00000000..24b699f2 --- /dev/null +++ b/src/Phaseolies/Console/Commands/SessionGcCommand.php @@ -0,0 +1,39 @@ +withTiming(function () { + $removed = SessionManager::store()->gc(SessionManager::config()['lifetime'] * 60); + + $this->info("Removed {$removed} expired session(s)."); + + return Command::SUCCESS; + }, 'Expired sessions have been removed.'); + } +} diff --git a/src/Phaseolies/Error/WebErrorRenderer.php b/src/Phaseolies/Error/WebErrorRenderer.php index 1813dcb1..55e1af93 100644 --- a/src/Phaseolies/Error/WebErrorRenderer.php +++ b/src/Phaseolies/Error/WebErrorRenderer.php @@ -85,8 +85,10 @@ public function renderDebug(Throwable $exception): Response 'current_route_name' => \Phaseolies\Support\Facades\Route::currentRouteName(), 'current_route_action' => \Phaseolies\Support\Facades\Route::currentRouteAction(), 'current_route_params' => request()->getRouteParams() - ]); + ], true); + // Rendered to a string (not echoed) so the status code and headers of + // the response are still applied when it is sent. return response($content, $this->resolveStatusCode($exception)) ->setOriginal($content) ->setStatusCode($this->resolveStatusCode($exception)); diff --git a/src/Phaseolies/Helpers/helpers.php b/src/Phaseolies/Helpers/helpers.php index 4251dd46..50234bcb 100644 --- a/src/Phaseolies/Helpers/helpers.php +++ b/src/Phaseolies/Helpers/helpers.php @@ -316,7 +316,7 @@ function session($key = null, $default = null): mixed */ function csrf_token(): ?string { - return session('_token') ?? null; + return session()->token(); } } diff --git a/src/Phaseolies/Http/Request.php b/src/Phaseolies/Http/Request.php index 42263f0d..8a914d8b 100644 --- a/src/Phaseolies/Http/Request.php +++ b/src/Phaseolies/Http/Request.php @@ -4,6 +4,7 @@ use Phaseolies\Utilities\IpUtils; use Phaseolies\Support\Session; +use Phaseolies\Session\Contracts\CommittableSession; use Phaseolies\Session\Contracts\SessionInterface; use Phaseolies\Support\File; use Phaseolies\Support\Facades\Str; @@ -164,6 +165,10 @@ public function __construct( $this->session = $container->has(SessionInterface::class) ? $container->get(SessionInterface::class) : new Session(); + + if ($this->session instanceof CommittableSession) { + $this->session->attach($this); + } $this->files = $files; $this->content = $content; $this->requestUri = $this->getPath(); diff --git a/src/Phaseolies/Http/Response.php b/src/Phaseolies/Http/Response.php index 9f8265e6..f7814d90 100644 --- a/src/Phaseolies/Http/Response.php +++ b/src/Phaseolies/Http/Response.php @@ -541,6 +541,7 @@ public function getStatusCodeText(int $statusCode): string 416 => 'Range Not Satisfiable', 417 => 'Expectation Failed', 418 => 'I\'m a Teapot', + 419 => 'Page Expired', 421 => 'Misdirected Request', 422 => 'Unprocessable Entity', 423 => 'Locked', diff --git a/src/Phaseolies/Launchers/SessionLauncher.php b/src/Phaseolies/Launchers/SessionLauncher.php index 67cfef90..e517e434 100644 --- a/src/Phaseolies/Launchers/SessionLauncher.php +++ b/src/Phaseolies/Launchers/SessionLauncher.php @@ -2,7 +2,7 @@ namespace Phaseolies\Launchers; -use Phaseolies\Session\ConfigSession as Session; +use Phaseolies\Session\SessionManager; class SessionLauncher extends ServiceLauncher { @@ -13,7 +13,7 @@ class SessionLauncher extends ServiceLauncher */ public function register() { - Session::configAppSession(); + SessionManager::initialize(); } /** diff --git a/src/Phaseolies/Middleware/CsrfTokenMiddleware.php b/src/Phaseolies/Middleware/CsrfTokenMiddleware.php index 752316e9..447f0204 100644 --- a/src/Phaseolies/Middleware/CsrfTokenMiddleware.php +++ b/src/Phaseolies/Middleware/CsrfTokenMiddleware.php @@ -2,13 +2,15 @@ namespace Phaseolies\Middleware; -use Phaseolies\Support\Facades\Str; -use Phaseolies\Middleware\Contracts\Middleware; -use Phaseolies\Http\Response\Cookie; -use Phaseolies\Http\Response; -use Phaseolies\Http\Request; -use Phaseolies\Http\Exceptions\TokenMismatchException; use Closure; +use Phaseolies\Http\Exceptions\HttpException; +use Phaseolies\Http\Exceptions\TokenMismatchException; +use Phaseolies\Http\Request; +use Phaseolies\Http\Response; +use Phaseolies\Http\Response\Cookie; +use Phaseolies\Middleware\Contracts\Middleware; +use Phaseolies\Session\Contracts\CommittableSession; +use Phaseolies\Support\Facades\Str; class CsrfTokenMiddleware implements Middleware { @@ -81,9 +83,13 @@ public function isTokenMatch($request): bool $token = $request->input('_token') ?: $request->header('X-CSRF-TOKEN'); - return is_string($request->session()->token()) && + // Read the stored token instead of token(), which would create one: + // a request that arrives without a session must fail, not start one. + $stored = $request->session()->get('_token'); + + return is_string($stored) && is_string($token) && - hash_equals($request->session()->token(), $token); + hash_equals($stored, $token); } /** @@ -99,6 +105,13 @@ protected function addCookieToResponse($request, $response): Response return $response; } + $session = $request->session(); + + // Stateless requests have no stored session to tie the token to. + if ($session instanceof CommittableSession && $session->isStateless()) { + return $response; + } + $config = config('session'); $response->headers->setCookie($this->addCookie($request, $config)); diff --git a/src/Phaseolies/Middleware/StatelessSession.php b/src/Phaseolies/Middleware/StatelessSession.php new file mode 100644 index 00000000..3edcc9d2 --- /dev/null +++ b/src/Phaseolies/Middleware/StatelessSession.php @@ -0,0 +1,30 @@ +session(); + + if ($session instanceof CommittableSession) { + $session->disable(); + } + + return $next($request); + } +} diff --git a/src/Phaseolies/Session/ConfigSession.php b/src/Phaseolies/Session/ConfigSession.php deleted file mode 100644 index 6b97fd44..00000000 --- a/src/Phaseolies/Session/ConfigSession.php +++ /dev/null @@ -1,22 +0,0 @@ -config = $config; - } - - /** - * Generates a CSRF token and stores it in the session if not already present. - * - * @return void - */ - public function generateToken(): void - { - $token = session('_token', bin2hex(random_bytes(16))) ?? null; - - if (!session()->has('_token')) { - session()->put('_token', $token); - } - } - - /** - * Regenerates the session ID to prevent session fixation attacks. - * - * @return void - */ - public function regenerate(): void - { - if (session_status() === PHP_SESSION_ACTIVE) { - session_regenerate_id(true); - } - } - - /** - * Determines if the session should be regenerated based on the configured lifetime. - * - * @return bool - */ - protected function shouldRegenerate(): bool - { - return !session()->has('last_regenerated') || - (time() - session('last_regenerated')) > ($this->config['lifetime'] * 60); - } -} diff --git a/src/Phaseolies/Session/Contracts/CommittableSession.php b/src/Phaseolies/Session/Contracts/CommittableSession.php new file mode 100644 index 00000000..df619249 --- /dev/null +++ b/src/Phaseolies/Session/Contracts/CommittableSession.php @@ -0,0 +1,46 @@ +configureCookieSession(); - $this->setCookieParameters(); - $this->validate(); - } - - /** - * Starts a session lifecycle, handles session ID regeneration, - * and ensures a session token is generated. - * - * This method checks if the session ID should be regenerated based on - * a configured interval. If regeneration is needed, it calls `session_regenerate_id()` - * and updates the `last_regenerated` timestamp in the session. - * Finally, it ensures a session token is generated. - */ - public function start(): void - { - if (session_status() === PHP_SESSION_NONE && !session_start()) { - throw new RuntimeException("Failed to start session."); - } - - if ($this->shouldRegenerate()) { - $this->regenerate(); - session()->put('last_regenerated', time()); - } - - $this->generateToken(); - } - - /** - * Configures PHP session INI settings to use cookie-based, user-defined session storage. - * - * This private method sets various `session.ini` directives to ensure - * sessions are handled via cookies and that the custom save handler - * methods (`open`, `close`, `read`, `write`, `destroy`, `gc`) are used. - * It configures security-related settings like `httponly`, `secure`, and `samesite`. - */ - private function configureCookieSession(): void {} - - /** - * Applies cookie parameters based on configuration. - * - * This method uses `session_set_cookie_params()` to set the detailed - * parameters for the session cookie, including its lifetime, path, domain, - * security flags (secure, httponly), and SameSite policy. - * It also sets the session name and the garbage collection max lifetime. - */ - private function setCookieParameters(): void - { - @ini_set('session.gc_maxlifetime', $this->config['lifetime'] * 60); - } - - /** - * Validates session cookie integrity; destroys cookie if it is invalid or cannot be decrypted. - * - * This method checks if a session cookie exists. If it does, it attempts to decrypt - * its content. If the decryption fails (e.g., due to tampering or incorrect key) - * or results in empty data, the session cookie is destroyed to prevent - * the use of invalid session data. - */ - public function validate(): void - { - if (isset($_COOKIE[$this->config['cookie']])) { - try { - if (empty($this->decrypt($_COOKIE[$this->config['cookie']]))) { - $this->destroyCookie(); - } - } catch (RuntimeException $e) { - $this->destroyCookie(); - } - } - } - - /** - * Called when a session is opened. - * - * @param string $savePath - * @param string $sessionName - * @return bool - */ - public function open($savePath, $sessionName): bool - { - return true; - } - - /** - * Called when a session is closed. - * - * @return bool - */ - public function close(): bool - { - return true; - } - - /** - * Reads session data from the cookie, decrypting it. - * - * @param string $sessionId - * @return string - */ - public function read($sessionId): string - { - if (!isset($_COOKIE[@session_name()])) { - return ''; - } - - try { - $data = $this->decrypt($_COOKIE[@session_name()]); - if (empty($data)) { - throw new RuntimeException('Empty decrypted session data'); - } - - return $data; - } catch (RuntimeException $e) { - $this->destroyCookie(); - return ''; - } - } - - /** - * Writes session data to the cookie, encrypting it. - * - * @param string $sessionId - * @param string $sessionData - * @return bool - */ - public function write($sessionId, $sessionData): bool - { - if (empty($sessionData)) { - return true; - } - - try { - $params = session_get_cookie_params(); - $encrypted = $this->encrypt($sessionData); - - $result = setcookie( - session_name(config('session.cookie')), - $encrypted, - [ - 'expires' => $params['lifetime'] ? time() + $params['lifetime'] : 0, - 'path' => $params['path'], - 'domain' => $params['domain'], - 'secure' => $params['secure'], - 'httponly' => $params['httponly'], - 'samesite' => $params['samesite'], - ] - ); - - if (!$result) { - return false; - } - - $_COOKIE[session_name(config('session.cookie'))] = $encrypted; - - return true; - } catch (RuntimeException $e) { - return false; - } - } - - /** - * Destroys the session data by invalidating the session cookie. - * - * @param string $sessionId - * @return bool - */ - public function destroy($sessionId): bool - { - return $this->destroyCookie(); - } - - /** - * Performs garbage collection for session data. - * - * @param int $maxlifetime - * @return bool - */ - public function gc($maxlifetime): bool - { - return true; - } - - /** - * Destroys the session cookie by setting its expiration to a past time. - * - * @return bool - */ - private function destroyCookie(): bool - { - if (headers_sent()) { - return false; - } - - setcookie($this->config['cookie'], '', [ - 'expires' => time() - 3600, - 'path' => $this->config['path'], - 'domain' => $this->config['domain'], - 'secure' => $this->config['secure'], - 'httponly' => true, - ]); - - unset($_COOKIE[$this->config['cookie']]); - - return true; - } - - /** - * Encrypts the given data using AES-256-CBC encryption. - * - * @param string $data - * @return string - * @throws RuntimeException - */ - private function encrypt(string $data): string - { - $key = Config::get('app.key'); - $iv = openssl_random_pseudo_bytes(openssl_cipher_iv_length('aes-256-cbc')); - $encrypted = openssl_encrypt($data, 'aes-256-cbc', $key, 0, $iv); - - return base64_encode($iv . $encrypted); - } - - /** - * Decrypts the given base64-encoded encrypted data using AES-256-CBC decryption. - * - * @param string $data - * @return string - * @throws RuntimeException - */ - private function decrypt(string $data): string - { - $key = Config::get('app.key'); - - // Ensure key is not null to avoid deprecation - if ($key === null) { - throw new RuntimeException('Encryption key not configured'); - } - - $data = base64_decode($data); - $ivSize = openssl_cipher_iv_length('aes-256-cbc'); - $iv = substr($data, 0, $ivSize); - $encrypted = substr($data, $ivSize); - - // Ensure IV is the correct length (16 bytes for aes-256-cbc) - if (strlen($iv) < $ivSize) { - $iv = str_pad($iv, $ivSize, "\0"); - } - - return openssl_decrypt($encrypted, 'aes-256-cbc', $key, 0, $iv); - } -} diff --git a/src/Phaseolies/Session/Handlers/FileSessionHandler.php b/src/Phaseolies/Session/Handlers/FileSessionHandler.php deleted file mode 100644 index 1a547cce..00000000 --- a/src/Phaseolies/Session/Handlers/FileSessionHandler.php +++ /dev/null @@ -1,80 +0,0 @@ -ensureSessionDirectoryExists(); - $this->configureFileSession(); - } - - /** - * Starts the session, applies custom session cache settings, - * and performs session regeneration and token generation as needed. - * - * @throws RuntimeException if the session fails to start. - */ - public function start(): void - { - // Disable PHP's default session cache headers - // (e.g., "Cache-Control: no-store, no-cache" and "Pragma: no-cache") - // This allows custom cache headers (like those set by doppar middleware) - // To take effect without being overridden - if (session_status() === PHP_SESSION_NONE) { - session_cache_limiter(''); - } - - if (session_status() === PHP_SESSION_NONE && !session_start()) { - throw new RuntimeException("Failed to start session."); - } - - if ($this->shouldRegenerate()) { - $this->regenerate(); - session()->put('last_regenerated', time()); - } - - $this->generateToken(); - } - - /** - * Ensures the session save directory exists. - * If it doesn't, attempts to create it. Fails with an exception if creation fails. - */ - private function ensureSessionDirectoryExists(): void - { - if (!is_dir($this->config['files'])) { - if (!mkdir($this->config['files'], 0700, true)) { - throw new RuntimeException("Failed to create session directory: {$this->config['files']}"); - } - } - } - - /** - * Configures PHP to use the filesystem for session storage - * with specific path and lifetime settings pulled from config. - */ - private function configureFileSession(): void - { - if (session_status() === PHP_SESSION_NONE) { - ini_set('session.save_handler', 'files'); - ini_set('session.save_path', $this->config['files']); - ini_set('session.gc_maxlifetime', $this->config['lifetime'] * 60); - } - } - - /** - * Placeholder for session validation logic. - * This could be expanded to implement custom validation checks. - */ - public function validate(): void {} -} diff --git a/src/Phaseolies/Session/Handlers/SessionHandlerFactory.php b/src/Phaseolies/Session/Handlers/SessionHandlerFactory.php deleted file mode 100644 index 3ebc9e49..00000000 --- a/src/Phaseolies/Session/Handlers/SessionHandlerFactory.php +++ /dev/null @@ -1,27 +0,0 @@ - new CookieSessionHandler($config), - 'file' => new FileSessionHandler($config), - default => throw new \RuntimeException("Unsupported session driver: {$driver}"), - }; - } -} diff --git a/src/Phaseolies/Session/Handlers/SessionManager.php b/src/Phaseolies/Session/Handlers/SessionManager.php deleted file mode 100644 index 833e89a2..00000000 --- a/src/Phaseolies/Session/Handlers/SessionManager.php +++ /dev/null @@ -1,65 +0,0 @@ - $config['expire_on_close'] ? 0 : $config['lifetime'] * 60, - 'path' => $config['path'], - 'domain' => $config['domain'], - 'secure' => $config['secure'], - 'httponly' => $config['http_only'], - 'samesite' => $config['same_site'] - ]); - - self::$handler = SessionHandlerFactory::create($config['driver'], $config); - self::$handler->initialize(); - self::$handler->start(); - } - - /** - * Returns the currently initialized session handler. - * - * @return SessionHandlerInterface - * @throws \RuntimeException - */ - public static function getHandler(): SessionHandlerInterface - { - if (self::$handler === null) { - throw new \RuntimeException("Session handler not initialized"); - } - - return self::$handler; - } -} diff --git a/src/Phaseolies/Session/SessionManager.php b/src/Phaseolies/Session/SessionManager.php new file mode 100644 index 00000000..cbf4110a --- /dev/null +++ b/src/Phaseolies/Session/SessionManager.php @@ -0,0 +1,165 @@ + 'file', + 'lifetime' => 120, + 'expire_on_close' => false, + 'files' => null, + 'cookie' => 'doppar_session', + 'path' => '/', + 'domain' => null, + 'secure' => null, + 'http_only' => true, + 'same_site' => 'lax', + 'stateless' => [], + 'lottery' => [2, 100], + ]; + + /** + * The resolved store, shared by every session of the process. + * + * @var SessionStoreInterface|null + */ + private static ?SessionStoreInterface $store = null; + + /** + * Custom drivers registered through extend(). + * + * @var array + */ + private static array $drivers = []; + + /** + * Prepare the session system for a new application boot. + * + * @return void + */ + public static function initialize(): void + { + self::$store = null; + } + + /** + * The session configuration with every default filled in. + * + * @return array + */ + public static function config(): array + { + $config = self::normalize((array) config('session')); + + $config['files'] ??= base_path('storage/framework/sessions'); + + return $config; + } + + /** + * Fill in defaults and normalize a session configuration array. + * + * @param array $config + * @return array + */ + public static function normalize(array $config): array + { + $config = array_replace(self::DEFAULTS, $config); + + $config['lifetime'] = max(1, (int) $config['lifetime']); + $config['cookie'] = self::cookieName((string) $config['cookie']); + + return $config; + } + + /** + * Make a cookie name safe to send and to read back. + * + * PHP rewrites dots and spaces in incoming cookie names to underscores, so + * a name such as "my app.session" is sent under one name and arrives under + * another, and the session would never be found. A name built from APP_NAME + * can easily contain either, so normalize it to a portable token. + * + * @param string $name + * @return string + */ + public static function cookieName(string $name): string + { + $name = preg_replace('/[^A-Za-z0-9_\-]+/', '_', trim($name)) ?? ''; + + return $name === '' ? self::DEFAULTS['cookie'] : $name; + } + + /** + * Get the store sessions are persisted in. + * + * @return SessionStoreInterface + * @throws SessionException + */ + public static function store(): SessionStoreInterface + { + return self::$store ??= self::createStore(self::config()); + } + + /** + * Replace the store, mainly for tests. + * + * @param SessionStoreInterface|null $store + * @return void + */ + public static function useStore(?SessionStoreInterface $store): void + { + self::$store = $store; + } + + /** + * Register a custom session driver. + * + * @param string $driver + * @param Closure(array): SessionStoreInterface $factory + * @return void + */ + public static function extend(string $driver, Closure $factory): void + { + self::$drivers[$driver] = $factory; + self::$store = null; + } + + /** + * Build the store for the configured driver. + * + * @param array $config + * @return SessionStoreInterface + * @throws SessionException + */ + public static function createStore(array $config): SessionStoreInterface + { + $driver = (string) $config['driver']; + $lifetime = $config['lifetime'] * 60; + + if (isset(self::$drivers[$driver])) { + return (self::$drivers[$driver])($config); + } + + return match ($driver) { + 'file' => new FileStore($config['files'], $lifetime), + 'cookie' => new CookieStore(app(EncryptionInterface::class), $lifetime), + 'array' => new ArrayStore($lifetime), + default => throw new SessionException('Unsupported session driver. Supported drivers: file, cookie, array.'), + }; + } +} diff --git a/src/Phaseolies/Session/Stores/ArrayStore.php b/src/Phaseolies/Session/Stores/ArrayStore.php new file mode 100644 index 00000000..58d8f60e --- /dev/null +++ b/src/Phaseolies/Session/Stores/ArrayStore.php @@ -0,0 +1,138 @@ + + */ + protected array $sessions = []; + + /** + * @param int $lifetime Seconds + */ + public function __construct(protected int $lifetime = 7200) + { + // + } + + /** + * Load a session from the value of the session cookie + * + * @param string $cookieValue + * @return array{id: string, data: array}|null + */ + public function read(string $cookieValue): ?array + { + $session = $this->sessions[$cookieValue] ?? null; + + if ($session === null) { + return null; + } + + if ($session['expires'] < time()) { + unset($this->sessions[$cookieValue]); + + return null; + } + + return ['id' => $cookieValue, 'data' => $session['data']]; + } + + /** + * Persist a session and return the value the session cookie must carry + * + * @param string $id + * @param array $data + * @return string + */ + public function write(string $id, array $data): string + { + $this->sessions[$id] = ['data' => $data, 'expires' => time() + $this->lifetime]; + + return $id; + } + + /** + * Refresh the lifetime of a session without rewriting its data + * + * @param string $id + * @return void + */ + public function touch(string $id): void + { + if (isset($this->sessions[$id])) { + $this->sessions[$id]['expires'] = time() + $this->lifetime; + } + } + + /** + * Remove a session + * + * @param string $id + * @return void + */ + public function destroy(string $id): void + { + unset($this->sessions[$id]); + } + + /** + * Remove expired sessions + * + * @param int $maxLifetime + * @return int + */ + public function gc(int $maxLifetime): int + { + $removed = 0; + + foreach ($this->sessions as $id => $session) { + if ($session['expires'] < time()) { + unset($this->sessions[$id]); + $removed++; + } + } + + return $removed; + } + + /** + * Remove expired sessions, doing only a bounded slice of the work + * + * @param int $maxLifetime + * @return int + */ + public function sweep(int $maxLifetime): int + { + return $this->gc($maxLifetime); + } + + /** + * Remove every stored session + * + * @return int + */ + public function flush(): int + { + $count = count($this->sessions); + $this->sessions = []; + + return $count; + } + + /** + * Whether the session data lives in the cookie instead of on the server + * + * @return bool + */ + public function isClientSide(): bool + { + return false; + } +} diff --git a/src/Phaseolies/Session/Stores/CookieStore.php b/src/Phaseolies/Session/Stores/CookieStore.php new file mode 100644 index 00000000..216825db --- /dev/null +++ b/src/Phaseolies/Session/Stores/CookieStore.php @@ -0,0 +1,179 @@ +encrypter->decrypt($cookieValue); + } catch (\Throwable) { + return null; + } + + if ( + !is_array($decoded) || + !isset($decoded['id'], $decoded['exp'], $decoded['data']) || + !is_string($decoded['id']) || + !FileStore::isValidId($decoded['id']) || + !is_array($decoded['data']) || + !is_int($decoded['exp']) || + $decoded['exp'] < time() + ) { + return null; + } + + return ['id' => $decoded['id'], 'data' => $decoded['data']]; + } + + /** + * Persist a session and return the value the session cookie must carry + * + * @param string $id + * @param array $data + * @return string + * @throws SessionException + */ + public function write(string $id, array $data): string + { + $payload = ['id' => $id, 'exp' => time() + $this->lifetime, 'data' => $data]; + + if (json_encode($payload) === false) { + throw new SessionException('Session data cannot be stored in a cookie because it is not JSON serializable.'); + } + + $value = $this->encrypter->encrypt($payload); + + if (strlen($value) > self::MAX_COOKIE_BYTES) { + throw new SessionException(sprintf( + 'Session data is too large for the cookie session driver (%d of %d bytes). Largest keys: %s. Store less in the session or use the file driver.', + strlen($value), + self::MAX_COOKIE_BYTES, + $this->largestKeys($data) + )); + } + + return $value; + } + + /** + * Describe the keys that take up the most room, by name and size only. + * + * @param array $data + * @return string + */ + protected function largestKeys(array $data): string + { + $sizes = array_map(static fn($value): int => strlen((string) json_encode($value)), $data); + + arsort($sizes); + + return implode(', ', array_map( + static fn($key, $size): string => "{$key} ({$size}B)", + array_keys($top = array_slice($sizes, 0, 3, true)), + $top + )); + } + + /** + * Refresh the lifetime of a session without rewriting its data + * + * @param string $id + * @return void + */ + public function touch(string $id): void + { + // + } + + /** + * Remove a session + * + * @param string $id + * @return void + */ + public function destroy(string $id): void + { + // + } + + /** + * Remove expired sessions + * + * @param int $maxLifetime + * @return int + */ + public function gc(int $maxLifetime): int + { + return 0; + } + + /** + * Remove expired sessions, doing only a bounded slice of the work + * + * @param int $maxLifetime + * @return int + */ + public function sweep(int $maxLifetime): int + { + return 0; + } + + /** + * Remove every stored session + * + * @return int + */ + public function flush(): int + { + return 0; + } + + /** + * Whether the session data lives in the cookie instead of on the server + * + * @return bool + */ + public function isClientSide(): bool + { + return true; + } +} diff --git a/src/Phaseolies/Session/Stores/FileStore.php b/src/Phaseolies/Session/Stores/FileStore.php new file mode 100644 index 00000000..c0a6839d --- /dev/null +++ b/src/Phaseolies/Session/Stores/FileStore.php @@ -0,0 +1,325 @@ +path = ($trimmed === '' || str_ends_with($trimmed, ':')) ? $path : $trimmed; + } + + /** + * Load a session from the value of the session cookie + * + * @param string $cookieValue + * @return array{id: string, data: array}|null + */ + public function read(string $cookieValue): ?array + { + if (!self::isValidId($cookieValue)) { + return null; + } + + $file = $this->file($cookieValue); + + // No lock is taken: writes replace the file atomically, so a reader + // sees either the old or the new session. File locks are mandatory on + // Windows and would only make readers and writers collide. + // PHP caches stat results; long-lived workers must not see stale times. + clearstatcache(true, $file); + + $modified = @filemtime($file); + $payload = $modified === false ? false : @file_get_contents($file); + + if ($payload === false) { + return null; + } + + if ($modified + $this->lifetime < time()) { + @unlink($file); + + return null; + } + + $data = $payload === '' ? null : @unserialize($payload); + + if (!is_array($data)) { + // Empty, corrupted, or caught mid-write on a platform where the + // write is not atomic: behave like an unknown session. The file is + // left for gc() so a live session is never deleted by a racing read. + return null; + } + + return ['id' => $cookieValue, 'data' => $data]; + } + + /** + * Persist a session and return the value the session cookie must carry + * + * @param string $id + * @param array $data + * @return string + * @throws SessionException + */ + public function write(string $id, array $data): string + { + $this->assertValidId($id); + + $file = $this->file($id); + $directory = dirname($file); + + if (!is_dir($directory) && !@mkdir($directory, 0700, true) && !is_dir($directory)) { + throw new SessionException('Unable to create the session storage directory.'); + } + + // Write to a private temp file and rename it over the target. Rename is + // atomic, so a concurrent reader sees the old or the new session, never + // a half-written one. + $temp = $directory . '/.tmp_' . bin2hex(random_bytes(8)); + + if (@file_put_contents($temp, serialize($data), LOCK_EX) === false) { + @unlink($temp); + + throw new SessionException('Unable to write the session.'); + } + + // Permissions are a no-op on Windows, where the profile directory ACLs apply. + @chmod($temp, 0600); + + if (!$this->replace($temp, $file)) { + @unlink($temp); + + throw new SessionException('Unable to write the session.'); + } + + return $id; + } + + /** + * Refresh the lifetime of a session without rewriting its data + * + * @param string $id + * @return void + */ + public function touch(string $id): void + { + if (!self::isValidId($id)) { + return; + } + + $file = $this->file($id); + + // Expiry is the file's mtime, so skip the write while it is still fresh. + clearstatcache(true, $file); + + $modified = @filemtime($file); + + if ($modified !== false && time() - $modified > min(60, intdiv($this->lifetime, 10))) { + @touch($file); + } + } + + /** + * Remove a session + * + * @param string $id + * @return void + */ + public function destroy(string $id): void + { + if (!self::isValidId($id)) { + return; + } + + $file = $this->file($id); + + if (@unlink($file) || !file_exists($file)) { + return; + } + + // Windows refuses to delete a file another process has open. The + // session must still stop working, so empty it: an empty file reads + // as an unknown session and gc() removes it later. + @file_put_contents($file, ''); + } + + /** + * Remove every expired session + * + * @param int $maxLifetime + * @return int + */ + public function gc(int $maxLifetime): int + { + $removed = 0; + $cutoff = time() - $maxLifetime; + + if (!is_dir($this->path)) { + return 0; + } + + // Plain directory iteration instead of glob(): a storage path that + // contains [ ] { } * or ? would make glob() match nothing. + foreach (new \FilesystemIterator($this->path, \FilesystemIterator::SKIP_DOTS) as $entry) { + $name = $entry->getFilename(); + + if ($entry->isDir() && preg_match('/^[a-f0-9]{2}$/', $name) === 1) { + $removed += $this->sweepDirectory($entry->getPathname(), $cutoff); + } elseif ($entry->isFile() && str_starts_with($name, 'sess_') && $entry->getMTime() <= $cutoff && @unlink($entry->getPathname())) { + // Left behind by the native PHP session handler this store replaced. + $removed++; + } + } + + return $removed; + } + + /** + * Remove expired sessions from one random shard + * + * @param int $maxLifetime + * @return int + */ + public function sweep(int $maxLifetime): int + { + return $this->sweepDirectory( + $this->path . '/' . sprintf('%02x', random_int(0, 255)), + time() - $maxLifetime + ); + } + + /** + * Remove every stored session + * + * @return int + */ + public function flush(): int + { + return $this->gc(-3600); + } + + /** + * Whether the session data lives in the cookie instead of on the server + * + * @return bool + */ + public function isClientSide(): bool + { + return false; + } + + /** + * Whether the given value is a well formed session id. + * + * @param string $id + * @return bool + */ + public static function isValidId(string $id): bool + { + return preg_match(self::ID_PATTERN, $id) === 1; + } + + /** + * Delete the files of one directory last modified before the cutoff. + * + * @param string $directory + * @param int $cutoff + * @return int + */ + protected function sweepDirectory(string $directory, int $cutoff): int + { + $removed = 0; + + if (!is_dir($directory)) { + return 0; + } + + foreach (new \FilesystemIterator($directory, \FilesystemIterator::SKIP_DOTS) as $file) { + if ($file->isFile() && $file->getMTime() <= $cutoff && @unlink($file->getPathname())) { + $removed++; + } + } + + return $removed; + } + + /** + * Move a freshly written temporary file over the session file. + * + * rename() is atomic and replaces the target on every supported platform, + * but on Windows it can fail for a moment while antivirus or another + * process holds the target open. Retry briefly, then fall back to writing + * in place, which is not atomic but never loses the session. + * + * @param string $temp + * @param string $file + * @return bool + */ + protected function replace(string $temp, string $file): bool + { + for ($attempt = 0; $attempt < 3; $attempt++) { + if ($this->move($temp, $file)) { + return true; + } + + usleep(10000 * ($attempt + 1)); + } + + $payload = @file_get_contents($temp); + @unlink($temp); + + return $payload !== false && @file_put_contents($file, $payload, LOCK_EX) !== false; + } + + /** + * @param string $from + * @param string $to + * @return bool + */ + protected function move(string $from, string $to): bool + { + return @rename($from, $to); + } + + /** + * Absolute path of the file backing a session. + * + * @param string $id + * @return string + */ + protected function file(string $id): string + { + return $this->path . '/' . substr($id, 0, 2) . '/' . $id; + } + + /** + * @param string $id + * @return void + * @throws SessionException + */ + protected function assertValidId(string $id): void + { + if (!self::isValidId($id)) { + throw new SessionException('Invalid session id.'); + } + } +} diff --git a/src/Phaseolies/Support/Session.php b/src/Phaseolies/Support/Session.php index fea8188f..60f58b9d 100644 --- a/src/Phaseolies/Support/Session.php +++ b/src/Phaseolies/Support/Session.php @@ -2,10 +2,24 @@ namespace Phaseolies\Support; +use Phaseolies\Http\Request; +use Phaseolies\Http\Response; +use Phaseolies\Http\Response\Cookie; +use Phaseolies\Session\Contracts\CommittableSession; use Phaseolies\Session\Contracts\SessionInterface; +use Phaseolies\Session\Contracts\SessionStoreInterface; +use Phaseolies\Session\SessionManager; +use Phaseolies\Session\Stores\FileStore; -class Session implements SessionInterface +class Session implements SessionInterface, CommittableSession { + /** + * Keys maintained by the session itself that do not make it worth storing. + * + * @var array + */ + protected const INTERNAL_KEYS = ['last_regenerated' => true]; + /** * The session data. * @@ -27,15 +41,349 @@ class Session implements SessionInterface */ protected $flashKey = '_flash'; + /** + * Whether the stored session was looked up yet. + */ + protected bool $loaded = false; + + /** + * Whether persistence is switched off for this request. + */ + protected bool $disabled = false; + + /** + * The id of the session, generated on demand. + */ + protected ?string $id = null; + + /** + * The id the session is currently stored under, if it is stored. + */ + protected ?string $storedId = null; + + /** + * An id whose stored data must be removed when the session is committed. + */ + protected ?string $previousId = null; + + /** + * The serialized data as it was loaded, to detect changes. + */ + protected ?string $snapshot = null; + + /** + * Whether the cookie must be expired when the session is committed. + */ + protected bool $forgetCookie = false; + + /** + * The request the session belongs to. + */ + protected ?Request $request = null; + /** * Constructor. * - * @param array|null $data + * @param SessionStoreInterface|null $store + * @param array|null $config + */ + public function __construct( + protected ?SessionStoreInterface $store = null, + protected ?array $config = null + ) { + if ($this->config !== null) { + $this->config = SessionManager::normalize($this->config); + } + } + + /** + * Bind the session to the request it belongs to. + * + * @param Request $request + * @return void + */ + public function attach(Request $request): void + { + $this->request = $request; + } + + /** + * Turn persistence off for the rest of the request. + * + * @return void + */ + public function disable(): void + { + $this->disabled = true; + } + + /** + * Whether the session was loaded during this request. + * + * @return bool + */ + public function isLoaded(): bool + { + return $this->loaded; + } + + /** + * Whether this request is not allowed to persist a session. + * + * @return bool + */ + public function isStateless(): bool + { + if ($this->disabled) { + return true; + } + + $patterns = (array) ($this->config()['stateless'] ?? []); + + if ($patterns === [] || $this->request === null) { + return false; + } + + $path = trim((string) parse_url($this->request->getPath(), PHP_URL_PATH), '/'); + + foreach ($patterns as $pattern) { + $regex = str_replace('\\*', '.*', preg_quote(trim((string) $pattern, '/'), '#')); + + if (preg_match('#^' . $regex . '$#i', $path) === 1) { + return $this->disabled = true; + } + } + + return false; + } + + /** + * Persist the session and queue its cookie. + * + * @param Response|null $response + * @return void + */ + public function commit(?Response $response = null): void + { + if (!$this->loaded || $this->isStateless()) { + return; + } + + $store = $this->store(); + $config = $this->config(); + + if ($this->isBlank()) { + $stale = $this->previousId ?? $this->storedId; + + if ($stale !== null) { + $store->destroy($stale); + } + + if ($stale !== null || $this->forgetCookie || $this->incomingCookie() !== null) { + $this->queueCookie($response, $this->expiredCookie($config)); + } + + $this->storedId = $this->previousId = null; + + return; + } + + $this->id ??= self::newId(); + $this->data['last_regenerated'] ??= time(); + + $changed = $this->storedId !== $this->id || serialize($this->data) !== $this->snapshot; + + if ($changed || $store->isClientSide()) { + $value = $store->write($this->id, $this->data); + } else { + $store->touch($this->id); + $value = $this->id; + } + + if ($this->previousId !== null && $this->previousId !== $this->id) { + $store->destroy($this->previousId); + } + + $this->storedId = $this->id; + $this->previousId = null; + $this->snapshot = serialize($this->data); + + $this->queueCookie($response, $this->makeCookie($value, $config)); + + $this->sweep($store, $config); + } + + /** + * Look up the stored session the first time the session is used. + * + * @return void + */ + protected function load(): void + { + if ($this->loaded) { + return; + } + + $this->loaded = true; + + if (!$this->isStateless()) { + $value = $this->incomingCookie(); + $record = $value === null ? null : $this->store()->read($value); + + if ($record !== null) { + $this->id = $this->storedId = $record['id']; + $this->data = $record['data']; + $this->snapshot = serialize($this->data); + + if ($this->shouldRotate()) { + $this->regenerate(true); + } + } + } + + $this->peek = array_replace($this->data, $this->peek); + } + + /** + * Whether the session id is due for rotation. + * + * @return bool + */ + protected function shouldRotate(): bool + { + return !isset($this->data['last_regenerated']) + || (time() - (int) $this->data['last_regenerated']) > ($this->config()['lifetime'] * 60); + } + + /** + * Whether the session holds nothing worth storing. + * + * @return bool + */ + protected function isBlank(): bool + { + return array_diff_key($this->data, self::INTERNAL_KEYS) === []; + } + + /** + * The raw value of the session cookie sent by the client. + * + * @return string|null + */ + protected function incomingCookie(): ?string + { + $name = $this->config()['cookie']; + + $value = $this->request !== null + ? $this->request->cookies->get($name) + : ($_COOKIE[$name] ?? null); + + return is_string($value) && $value !== '' ? $value : null; + } + + /** + * Build the session cookie. + * + * @param string $value + * @param array $config + * @return Cookie + */ + protected function makeCookie(string $value, array $config): Cookie + { + $sameSite = strtolower((string) ($config['same_site'] ?: 'lax')); + $secure = $config['secure'] ?? ($this->request?->isSecure() ?? false); + + return new Cookie( + $config['cookie'], + $value, + $config['expire_on_close'] ? 0 : time() + $config['lifetime'] * 60, + $config['path'], + $config['domain'], + // Browsers reject SameSite=None without Secure. + $sameSite === 'none' ? true : (bool) $secure, + (bool) $config['http_only'], + false, + $sameSite + ); + } + + /** + * Build a cookie that makes the browser drop the session cookie. + * + * @param array $config + * @return Cookie + */ + protected function expiredCookie(array $config): Cookie + { + return new Cookie( + $config['cookie'], + '', + 1, + $config['path'], + $config['domain'], + (bool) ($config['secure'] ?? false), + (bool) $config['http_only'], + false, + strtolower((string) ($config['same_site'] ?: 'lax')) + ); + } + + /** + * Attach the cookie to the response, or send it as a header without one. + * + * @param Response|null $response + * @param Cookie $cookie + * @return void + */ + protected function queueCookie(?Response $response, Cookie $cookie): void + { + if ($response !== null) { + $response->headers->setCookie($cookie); + } elseif (!headers_sent()) { + header('Set-Cookie: ' . $cookie, false); + } + } + + /** + * Occasionally remove expired sessions. + * + * @param SessionStoreInterface $store + * @param array $config + * @return void + */ + protected function sweep(SessionStoreInterface $store, array $config): void + { + [$chances, $total] = array_pad(array_map('intval', (array) $config['lottery']), 2, 100); + + if ($chances > 0 && $total > 0 && random_int(1, $total) <= $chances) { + $store->sweep($config['lifetime'] * 60); + } + } + + /** + * Generate a session id. + * + * @return string + */ + protected static function newId(): string + { + return bin2hex(random_bytes(20)); + } + + /** + * @return SessionStoreInterface + */ + protected function store(): SessionStoreInterface + { + return $this->store ??= SessionManager::store(); + } + + /** + * @return array */ - public function __construct() + protected function config(): array { - $this->data = &$_SESSION; - $this->peek = $_SESSION; + return $this->config ??= SessionManager::config(); } /** @@ -47,13 +395,13 @@ public function __construct() */ public function get(string $key, $default = null) { - $value = $this->data[$key] ?? $default; + $this->load(); - return $value; + return $this->data[$key] ?? $default; } /** - * Get a session value by key. + * Get a session value by key and remove it. * * @param string $key * @param mixed $default @@ -61,7 +409,7 @@ public function get(string $key, $default = null) */ public function pull(string $key, $default = null) { - $value = $this->data[$key] ?? $default; + $value = $this->get($key, $default); $this->forget($key); @@ -69,7 +417,7 @@ public function pull(string $key, $default = null) } /** - * Get a session value by key. + * Get a peek value by key and remove it from the session. * * @param string $key * @param mixed $default @@ -77,6 +425,8 @@ public function pull(string $key, $default = null) */ public function getPeek(string $key, $default = null) { + $this->load(); + $value = $this->peek[$key] ?? $default; $this->forget($key); @@ -85,7 +435,7 @@ public function getPeek(string $key, $default = null) } /** - * Put a key-value pair into the session. + * Put a key-value pair into the peek data. * * @param string $key * @param mixed $value @@ -115,6 +465,8 @@ public function flushPeek(): void */ public function put(string|array $key, $value = null): void { + $this->load(); + if (is_array($key)) { foreach ($key as $k => $v) { $this->data[$k] = $v; @@ -124,7 +476,6 @@ public function put(string|array $key, $value = null): void } } - /** * Check if a session key exists. * @@ -133,6 +484,8 @@ public function put(string|array $key, $value = null): void */ public function has(string $key): bool { + $this->load(); + return isset($this->data[$key]); } @@ -144,6 +497,8 @@ public function has(string $key): bool */ public function forget(string $key): void { + $this->load(); + unset($this->data[$key]); } @@ -154,6 +509,8 @@ public function forget(string $key): void */ public function flush(): void { + $this->load(); + $this->data = []; } @@ -165,7 +522,14 @@ public function flush(): void */ public function regenerate(bool $deleteOldSession = true): void { - session_regenerate_id($deleteOldSession); + $this->load(); + + if ($deleteOldSession && $this->storedId !== null) { + $this->previousId ??= $this->storedId; + } + + $this->id = self::newId(); + $this->data['last_regenerated'] = time(); } /** @@ -175,6 +539,8 @@ public function regenerate(bool $deleteOldSession = true): void */ public function all(): array { + $this->load(); + return $this->data; } @@ -185,7 +551,9 @@ public function all(): array */ public function getId(): string { - return session_id(); + $this->load(); + + return $this->id ??= self::newId(); } /** @@ -196,7 +564,21 @@ public function getId(): string */ public function setId(string $id): void { - session_id($id); + $this->load(); + + if (!FileStore::isValidId($id)) { + return; + } + + $store = $this->store(); + + if (!$store->isClientSide() && !$this->isStateless() && ($record = $store->read($id)) !== null) { + $this->data = $record['data']; + $this->storedId = $record['id']; + $this->snapshot = serialize($this->data); + } + + $this->id = $id; } /** @@ -206,20 +588,25 @@ public function setId(string $id): void */ public function destroy(): void { - session_destroy(); + $this->load(); + $this->previousId ??= $this->storedId; + $this->storedId = $this->id = $this->snapshot = null; + $this->forgetCookie = true; $this->data = []; $this->peek = []; } /** - * Get the CSRF token. + * Get the CSRF token, creating it the first time it is needed. * * @return string|null */ public function token(): ?string { - return $this->get('_token'); + $this->load(); + + return $this->data['_token'] ??= $this->generateToken(); } /** diff --git a/tests/Error/WebErrorRendererTest.php b/tests/Error/WebErrorRendererTest.php index fc9c93a9..15826cce 100644 --- a/tests/Error/WebErrorRendererTest.php +++ b/tests/Error/WebErrorRendererTest.php @@ -7,6 +7,7 @@ use Phaseolies\DI\Container; use Phaseolies\Error\WebErrorRenderer; use Phaseolies\Http\Exceptions\HttpException; +use Phaseolies\Http\Exceptions\TokenMismatchException; use Phaseolies\Http\Response; use PHPUnit\Framework\TestCase; use RuntimeException; @@ -45,4 +46,20 @@ public function testRenderProductionPreservesHttpExceptionStatus(): void $this->assertSame('Not Found', $response->getBody()); $this->assertSame($exception, $response->getOriginal()); } + + public function testRenderProductionKeepsTheStatusOfATokenMismatch(): void + { + $renderer = new WebErrorRenderer(); + $exception = new TokenMismatchException(419, 'CSRF Token mismatched'); + + $response = $renderer->renderProduction($exception); + + $this->assertSame(419, $response->getStatusCode()); + $this->assertSame('CSRF Token mismatched', $response->getBody()); + } + + public function testStatus419HasAReadableReasonPhrase(): void + { + $this->assertSame('Page Expired', (new Response())->getStatusCodeText(419)); + } } diff --git a/tests/Requests/RequestTest.php b/tests/Requests/RequestTest.php index 67b6685b..acd4c17a 100644 --- a/tests/Requests/RequestTest.php +++ b/tests/Requests/RequestTest.php @@ -3,6 +3,7 @@ namespace Tests\Unit\Requests; use Phaseolies\Support\Session; +use Phaseolies\Session\Stores\ArrayStore; use Phaseolies\Http\Request; use Phaseolies\Http\ParameterBag; use Phaseolies\Http\HeaderBag; @@ -342,17 +343,20 @@ public function testSetAndGetRouteParams() public function testSession() { + $config = ['cookie' => 'test_session', 'lifetime' => 120, 'stateless' => [], 'lottery' => [0, 100]]; + + $this->request->session = new Session(new ArrayStore(), $config); $session = $this->request->session(); + $session->put('user_id', 1); $this->assertEquals(1, $session->get('user_id')); $session->put('test_key', 'test_value'); $this->assertEquals('test_value', $session->get('test_key')); $emptyRequest = new Request(); - $_SESSION = []; - $emptySession = $emptyRequest->session(); - $this->assertEmpty($emptySession->all()); + $emptyRequest->session = new Session(new ArrayStore(), $config); + $this->assertEmpty($emptyRequest->session()->all()); } public function testCapture() diff --git a/tests/Requests/SessionTest.php b/tests/Requests/SessionTest.php index 97e34a0f..a7b46eb6 100644 --- a/tests/Requests/SessionTest.php +++ b/tests/Requests/SessionTest.php @@ -2,6 +2,7 @@ namespace Tests\Unit\Requests; +use Phaseolies\Session\Stores\ArrayStore; use Phaseolies\Support\Session; use Phaseolies\Session\Contracts\SessionInterface; use Phaseolies\Application; @@ -17,23 +18,24 @@ class SessionTest extends TestCase protected function setUp(): void { - // Mock the global $_SESSION - $_SESSION = []; - $this->mockSession = &$_SESSION; - - $this->session = new Session(); + $this->session = new Session(new ArrayStore(), [ + 'cookie' => 'test_session', + 'lifetime' => 120, + 'stateless' => [], + 'lottery' => [0, 100], + ]); } - protected function tearDown(): void + public function testConstructorInitializesAnEmptySession() { - // Clear the mock session after each test - $_SESSION = []; - parent::tearDown(); + $this->assertSame([], $this->session->all()); } - public function testConstructorInitializesSession() + public function testConstructorDoesNotTouchTheStoreOrConfig(): void { - $this->assertSame($_SESSION, $this->session->all()); + // No arguments, no application: creating a session must have no side + // effects until it is used. + $this->assertInstanceOf(Session::class, new Session()); } public function testSessionImplementationConformsToTheSessionContract(): void @@ -60,9 +62,6 @@ public function testApplicationBindsTheSessionContractAndLegacyServiceNameToTheS $this->assertInstanceOf(Session::class, $contractSession); $this->assertSame($contractSession, $legacySession); - $contractSession->put('from_contract', 'value'); - - $this->assertSame(['from_contract' => 'value'], $legacySession->all()); } public function testSessionIsResetBetweenRequestsAndOverridableByLegacyKey(): void @@ -77,7 +76,6 @@ public function testSessionIsResetBetweenRequestsAndOverridableByLegacyKey(): vo $app->snapshotBootBindings(); $first = $app->get('session'); - $first->put('user_id', 1); // Same sweep Application::cleanupRequestScopedServices() performs. $app->forgetResolved('session'); @@ -86,7 +84,6 @@ public function testSessionIsResetBetweenRequestsAndOverridableByLegacyKey(): vo $second = $app->get('session'); $this->assertNotSame($first, $second); - $second->put('user_id', 2); $app->forgetResolved('session'); $this->assertNotSame($second, $app->get('session')); @@ -99,7 +96,7 @@ public function testPutAndGetSingleValue() { $this->session->put('test_key', 'test_value'); $this->assertEquals('test_value', $this->session->get('test_key')); - $this->assertArrayHasKey('test_key', $_SESSION); + $this->assertArrayHasKey('test_key', $this->session->all()); } public function testPutAndGetArray() @@ -152,7 +149,6 @@ public function testFlush() $this->session->flush(); $this->assertEmpty($this->session->all()); - $this->assertEmpty($_SESSION); } public function testAll() @@ -178,12 +174,20 @@ public function testPeekOperations() public function testSetAndGetId() { - $newId = 'test_session_id'; + $newId = bin2hex(random_bytes(20)); $this->session->setId($newId); $this->assertEquals($newId, $this->session->getId()); } + public function testSetIdIgnoresAnIdThatWasNotGeneratedByTheFramework() + { + $before = $this->session->getId(); + $this->session->setId('test_session_id'); + + $this->assertSame($before, $this->session->getId()); + } + // public function testDestroy() // { // $this->session->put('key1', 'value1'); @@ -197,7 +201,10 @@ public function testSetAndGetId() public function testToken() { - $this->assertNull($this->session->token()); + $token = $this->session->token(); + + $this->assertSame(32, strlen($token)); + $this->assertSame($token, $this->session->token()); $this->session->put('_token', 'test_token'); $this->assertEquals('test_token', $this->session->token()); diff --git a/tests/Session/CookieSessionHandlerTest.php b/tests/Session/CookieSessionHandlerTest.php deleted file mode 100644 index 33314e0b..00000000 --- a/tests/Session/CookieSessionHandlerTest.php +++ /dev/null @@ -1,203 +0,0 @@ - 'doppar_session', - 'lifetime' => 120, - 'path' => '/', - 'domain' => '', - 'secure' => false, - 'same_site' => 'Lax', - ]; - - private function makeHandler(array $overrides = []): CookieSessionHandler - { - return new CookieSessionHandler(array_merge($this->config, $overrides)); - } - - protected function setUp(): void - { - Container::setInstance(new MockContainer()); - $this->seedConfig([ - 'app' => ['key' => str_repeat('a', 32)], - 'session' => ['cookie' => $this->config['cookie']], - ]); - session_name($this->config['cookie']); - } - - protected function tearDown(): void - { - unset($_COOKIE[$this->config['cookie']]); - Container::forgetInstance(); - } - - public function testOpenReturnsTrue() - { - $handler = $this->makeHandler(); - - $this->assertTrue($handler->open('/tmp', 'doppar_session')); - } - - public function testCloseReturnsTrue() - { - $handler = $this->makeHandler(); - - $this->assertTrue($handler->close()); - } - - public function testGcReturnsTrue() - { - $handler = $this->makeHandler(); - - $this->assertTrue($handler->gc(3600)); - } - - public function testReadReturnsEmptyStringWhenNoCookiePresent() - { - unset($_COOKIE['doppar_session']); - - $handler = $this->makeHandler(); - - $this->assertEquals('', $handler->read('session_id_123')); - } - - public function testReadReturnsEmptyStringForMissingSessionName() - { - foreach (array_keys($_COOKIE) as $key) { - unset($_COOKIE[$key]); - } - - $handler = $this->makeHandler(); - - $result = $handler->read('any_id'); - - $this->assertIsString($result); - $this->assertEquals('', $result); - } - - public function testValidateDestroysInvalidCookieRequiresConfig() - { - // Set up a cookie with invalid base64 data that will cause decryption to fail - $_COOKIE[$this->config['cookie']] = 'invalid_encrypted_data'; - - $handler = $this->makeHandler(); - - // Validate should handle the invalid cookie gracefully by destroying it - // Even if Config is not available, the method should not throw a fatal error - try { - $handler->validate(); - } catch (Exception $e) { - // If an exception occurs due to missing Config or key, that's expected in test environment - $this->assertStringContainsString('Config', $e->getMessage()); - $this->assertStringContainsString('key', $e->getMessage()); - } - - // Test that validate() exists and is callable - $this->assertTrue(method_exists($handler, 'validate')); - } - - public function testValidateDoesNothingWhenNoCookiePresent() - { - unset($_COOKIE[$this->config['cookie']]); - - $handler = $this->makeHandler(); - - $handler->validate(); - - $this->assertArrayNotHasKey($this->config['cookie'], $_COOKIE); - } - - public function testHandlerIsInstantiableWithValidConfig() - { - $handler = $this->makeHandler(); - - $this->assertInstanceOf(CookieSessionHandler::class, $handler); - } - - public function testWriteReturnsTrueForEmptyData() - { - $handler = $this->makeHandler(); - - $this->assertTrue($handler->write('session_id', '')); - } - - public function testDestroyReturnsTrueWhenHeadersNotSent() - { - $handler = $this->makeHandler(); - - $result = $handler->destroy('session_id_456'); - - $this->assertIsBool($result); - } - - public function testPrivateEncryptAndDecryptRoundTrip() - { - $handler = $this->makeHandler(); - $encrypt = new \ReflectionMethod($handler, 'encrypt'); - $decrypt = new \ReflectionMethod($handler, 'decrypt'); - - $encrypted = $encrypt->invoke($handler, 'session_payload'); - $decrypted = $decrypt->invoke($handler, $encrypted); - - $this->assertIsString($encrypted); - $this->assertNotSame('session_payload', $encrypted); - $this->assertSame('session_payload', $decrypted); - } - - public function testValidatePreservesValidEncryptedCookie() - { - $handler = $this->makeHandler(); - $encrypt = new \ReflectionMethod($handler, 'encrypt'); - - $_COOKIE[$this->config['cookie']] = $encrypt->invoke($handler, 'valid_payload'); - - $handler->validate(); - - $this->assertArrayHasKey($this->config['cookie'], $_COOKIE); - } - - public function testDecryptThrowsWhenAppKeyIsMissing() - { - $this->seedConfig([ - 'app' => ['key' => null], - 'session' => ['cookie' => $this->config['cookie']], - ]); - $handler = $this->makeHandler(); - $decrypt = new \ReflectionMethod($handler, 'decrypt'); - - $this->expectException(RuntimeException::class); - $this->expectExceptionMessage('Encryption key not configured'); - - $decrypt->invoke($handler, base64_encode('anything')); - } - - private function seedConfig(array $config): void - { - $this->setConfigStatic('cacheFile', sys_get_temp_dir() . '/doppar_test_config.php'); - $this->setConfigStatic('config', $config); - $this->setConfigStatic('loadedFromCache', true); - $this->setConfigStatic('configModified', false); - $this->setConfigStatic('fileHashes', []); - $this->setConfigStatic('configFiles', []); - } - - private function setConfigStatic(string $propertyName, mixed $value): void - { - $reflection = new \ReflectionClass(Config::class); - $property = $reflection->getProperty($propertyName); - $property->setValue(null, $value); - } -} diff --git a/tests/Session/CookieStoreTest.php b/tests/Session/CookieStoreTest.php new file mode 100644 index 00000000..9a5a5d51 --- /dev/null +++ b/tests/Session/CookieStoreTest.php @@ -0,0 +1,67 @@ +id(); + + $value = $store->write($id, ['user' => 7]); + + $this->assertStringNotContainsString('user', $value); + $this->assertSame(['id' => $id, 'data' => ['user' => 7]], $store->read($value)); + } + + public function test_it_rejects_tampered_garbage_and_legacy_values(): void + { + $store = new CookieStore(new Encryption()); + $value = $store->write($this->id(), ['user' => 7]); + + $this->assertNull($store->read(substr($value, 0, -4) . 'AAAA')); + $this->assertNull($store->read('garbage')); + $this->assertNull($store->read(base64_encode(random_bytes(64)))); + } + + public function test_it_rejects_expired_values(): void + { + $value = (new CookieStore(new Encryption(), -10))->write($this->id(), ['a' => 1]); + + $this->assertNull((new CookieStore(new Encryption()))->read($value)); + } + + public function test_it_refuses_to_emit_an_oversized_cookie(): void + { + $this->expectException(SessionException::class); + + (new CookieStore(new Encryption()))->write($this->id(), ['blob' => str_repeat('x', 5000)]); + } + + public function test_the_overflow_error_names_the_largest_keys_but_not_their_values(): void + { + try { + (new CookieStore(new Encryption()))->write($this->id(), ['small' => 'a', 'big_key' => str_repeat('secret', 900)]); + $this->fail('Expected an overflow.'); + } catch (SessionException $e) { + $this->assertStringContainsString('big_key', $e->getMessage()); + $this->assertStringNotContainsString('secret', $e->getMessage()); + } + } + + public function test_it_is_client_side(): void + { + $this->assertTrue((new CookieStore(new Encryption()))->isClientSide()); + } +} diff --git a/tests/Session/DispatchSessionTest.php b/tests/Session/DispatchSessionTest.php new file mode 100644 index 00000000..001537c2 --- /dev/null +++ b/tests/Session/DispatchSessionTest.php @@ -0,0 +1,147 @@ +app = (new ReflectionClass(Application::class))->newInstanceWithoutConstructor(); + $this->store = new ArrayStore(); + } + + private function request(): Request + { + $request = new Request(); + $request->session = new Session($this->store, [ + 'cookie' => self::COOKIE, + 'lifetime' => 120, + 'stateless' => [], + 'lottery' => [0, 100], + ]); + $request->session->attach($request); + + return $request; + } + + private function routeTo(callable $handler): void + { + $router = $this->createMock(Router::class); + $router->method('resolve')->willReturnCallback($handler); + $this->app->router = $router; + } + + private function savedCookie(Response $response): ?string + { + foreach ($response->headers->getCookies() as $cookie) { + if ($cookie->getName() === self::COOKIE) { + return $cookie->getValue(); + } + } + + return null; + } + + public function testASessionWrittenBeforeAValidationRedirectIsSavedOnThatRedirect(): void + { + // Validation failures throw an HttpResponseException that carries the + // redirect. The flashed errors and old input must survive it. + $redirect = new Response('', 302); + + $request = $this->request(); + $this->routeTo(function () use ($request, $redirect) { + $request->session()->put('errors', ['name' => ['The Name field is required']]); + $request->session()->put('input', ['name' => '']); + + throw (new HttpResponseException('invalid', 422))->setResponse($redirect); + }); + + try { + $this->app->dispatch($request); + $this->fail('The exception must still reach the error handler.'); + } catch (HttpResponseException $e) { + $this->assertSame($redirect, $e->getResponse()); + } + + $value = $this->savedCookie($redirect); + + $this->assertNotNull($value, 'The redirect must carry the session cookie.'); + $this->assertSame( + ['name' => ['The Name field is required']], + $this->store->read($value)['data']['errors'] + ); + } + + public function testAnHttpResponseExceptionWithoutASessionSendsNoCookie(): void + { + $redirect = new Response('', 302); + + $request = $this->request(); + $this->routeTo(fn () => throw (new HttpResponseException('x', 404))->setResponse($redirect)); + + try { + $this->app->dispatch($request); + } catch (HttpResponseException) { + } + + $this->assertNull($this->savedCookie($redirect)); + } + + public function testAnUnexpectedExceptionIsNotMaskedByTheSession(): void + { + $request = $this->request(); + $this->routeTo(function () use ($request) { + $request->session()->put('a', 1); + + throw new RuntimeException('boom'); + }); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('boom'); + + $this->app->dispatch($request); + } + + public function testAFailingSessionStoreNeverReplacesTheOriginalException(): void + { + $request = $this->request(); + $request->session = new Session(new class extends ArrayStore { + public function write(string $id, array $data): string + { + throw new RuntimeException('store down'); + } + }, ['cookie' => self::COOKIE, 'lifetime' => 120, 'stateless' => [], 'lottery' => [0, 100]]); + $request->session->attach($request); + + $redirect = new Response('', 302); + $this->routeTo(function () use ($request, $redirect) { + $request->session()->put('a', 1); + + throw (new HttpResponseException('invalid', 422))->setResponse($redirect); + }); + + $this->expectException(HttpResponseException::class); + + $this->app->dispatch($request); + } +} diff --git a/tests/Session/FileStoreTest.php b/tests/Session/FileStoreTest.php new file mode 100644 index 00000000..44cfe2c9 --- /dev/null +++ b/tests/Session/FileStoreTest.php @@ -0,0 +1,273 @@ +path = sys_get_temp_dir() . '/doppar_sessions_' . bin2hex(random_bytes(6)); + } + + protected function tearDown(): void + { + (new FileStore($this->path))->flush(); + + foreach (glob($this->path . '/*') ?: [] as $entry) { + is_dir($entry) ? @rmdir($entry) : @unlink($entry); + } + + @rmdir($this->path); + } + + private function id(string $prefix = 'ab'): string + { + return $prefix . bin2hex(random_bytes(19)); + } + + public function test_it_round_trips_session_data(): void + { + $store = new FileStore($this->path); + $id = $this->id(); + + $this->assertSame($id, $store->write($id, ['user' => 7, 'nested' => ['a' => 1]])); + + $this->assertSame( + ['id' => $id, 'data' => ['user' => 7, 'nested' => ['a' => 1]]], + $store->read($id) + ); + } + + public function test_it_shards_files_by_the_first_two_characters_of_the_id(): void + { + $store = new FileStore($this->path); + $id = $this->id('cd'); + $store->write($id, ['a' => 1]); + + $this->assertFileExists($this->path . '/cd/' . $id); + } + + public function test_session_files_are_private_to_the_application_user(): void + { + if (PHP_OS_FAMILY === 'Windows') { + $this->markTestSkipped('POSIX permissions do not apply on Windows.'); + } + + $store = new FileStore($this->path); + $id = $this->id('cd'); + $store->write($id, ['a' => 1]); + + $this->assertSame('0600', substr(sprintf('%o', fileperms($this->path . '/cd/' . $id)), -4)); + } + + public function test_it_rejects_ids_that_are_not_generated_ids(): void + { + $store = new FileStore($this->path); + $store->write($this->id(), ['a' => 1]); + + foreach (['../../etc/passwd', 'abc', str_repeat('A', 40), $this->id() . '/..', '', "ab\0" . str_repeat('c', 37)] as $bad) { + $this->assertNull($store->read($bad), "Accepted [$bad]"); + } + + $this->expectException(SessionException::class); + $store->write('../evil', ['a' => 1]); + } + + public function test_it_treats_unknown_expired_and_corrupt_sessions_as_missing(): void + { + $store = new FileStore($this->path, 60); + + $this->assertNull($store->read($this->id())); + + $expired = $this->id(); + $store->write($expired, ['a' => 1]); + touch($this->path . '/ab/' . $expired, time() - 120); + $this->assertNull($store->read($expired)); + $this->assertFileDoesNotExist($this->path . '/ab/' . $expired); + + $corrupt = $this->id(); + $store->write($corrupt, ['a' => 1]); + file_put_contents($this->path . '/ab/' . $corrupt, 'not-a-serialized-array'); + $this->assertNull($store->read($corrupt)); + + // A read must never delete a file that may be mid-write; gc() does that. + $this->assertFileExists($this->path . '/ab/' . $corrupt); + } + + public function test_touch_slides_the_expiry_and_destroy_removes_the_session(): void + { + $store = new FileStore($this->path, 60); + $id = $this->id(); + $store->write($id, ['a' => 1]); + + touch($this->path . '/ab/' . $id, time() - 50); + $store->touch($id); + $this->assertGreaterThan(time() - 5, filemtime($this->path . '/ab/' . $id)); + + $store->destroy($id); + $this->assertNull($store->read($id)); + } + + public function test_gc_removes_only_expired_sessions_and_legacy_native_files(): void + { + $store = new FileStore($this->path, 60); + $fresh = $this->id('aa'); + $old = $this->id('bb'); + $store->write($fresh, ['a' => 1]); + $store->write($old, ['a' => 1]); + touch($this->path . '/bb/' . $old, time() - 600); + + file_put_contents($this->path . '/sess_legacy', 'x'); + touch($this->path . '/sess_legacy', time() - 600); + + $this->assertSame(2, $store->gc(60)); + $this->assertNotNull($store->read($fresh)); + $this->assertNull($store->read($old)); + $this->assertFileDoesNotExist($this->path . '/sess_legacy'); + } + + public function test_sweep_is_bounded_to_a_single_shard(): void + { + $store = new FileStore($this->path, 60); + + // Fill every shard with one expired session, then sweep once. + for ($i = 0; $i < 256; $i++) { + $id = sprintf('%02x', $i) . bin2hex(random_bytes(19)); + $store->write($id, ['a' => 1]); + touch($this->path . '/' . substr($id, 0, 2) . '/' . $id, time() - 600); + } + + $this->assertLessThanOrEqual(1, $store->sweep(60)); + $this->assertGreaterThanOrEqual(255, count(glob($this->path . '/*/*'))); + } + + public function test_flush_removes_everything(): void + { + $store = new FileStore($this->path); + $store->write($this->id('aa'), ['a' => 1]); + $store->write($this->id('bb'), ['a' => 1]); + + $this->assertSame(2, $store->flush()); + $this->assertSame([], glob($this->path . '/*/*') ?: []); + } + + public function test_writes_leave_no_temporary_files_behind(): void + { + $store = new FileStore($this->path); + $id = $this->id(); + + for ($i = 0; $i < 20; $i++) { + $store->write($id, ['i' => $i]); + } + + $files = array_values(array_diff(scandir($this->path . '/ab'), ['.', '..'])); + + $this->assertSame([$id], $files); + $this->assertSame(19, $store->read($id)['data']['i']); + } + + public function test_a_storage_path_containing_glob_characters_still_works(): void + { + // [ ] { } * ? are legal in directory names on every OS and make glob() match nothing. + $path = $this->path . '/we[ir]d {dir} *?'; + $store = new FileStore($path, 60); + + $fresh = $this->id('aa'); + $old = $this->id('bb'); + $store->write($fresh, ['a' => 1]); + $store->write($old, ['a' => 1]); + touch($path . '/bb/' . $old, time() - 600); + file_put_contents($path . '/sess_legacy', 'x'); + touch($path . '/sess_legacy', time() - 600); + + $this->assertSame(2, $store->gc(60)); + $this->assertNotNull($store->read($fresh)); + $this->assertSame(1, $store->flush()); + $this->assertNull($store->read($fresh)); + + foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path, \FilesystemIterator::SKIP_DOTS), \RecursiveIteratorIterator::CHILD_FIRST) as $entry) { + $entry->isDir() ? @rmdir($entry->getPathname()) : @unlink($entry->getPathname()); + } + + @rmdir($path); + } + + public function test_a_failing_rename_falls_back_to_writing_in_place(): void + { + // Windows can refuse rename() while antivirus or another process has + // the target open. The session must still be written, with no temp files left. + $store = new class($this->path) extends FileStore + { + public int $attempts = 0; + + protected function move(string $from, string $to): bool + { + $this->attempts++; + + return false; + } + }; + + $id = $this->id(); + $store->write($id, ['user' => 7]); + $store->write($id, ['user' => 8]); + + $this->assertSame(6, $store->attempts); + $this->assertSame(8, $store->read($id)['data']['user']); + $this->assertSame([$id], array_values(array_diff(scandir($this->path . '/ab'), ['.', '..']))); + } + + public function test_a_transient_rename_failure_is_retried(): void + { + $store = new class($this->path) extends FileStore + { + protected function move(string $from, string $to): bool + { + static $calls = 0; + + return ++$calls < 2 ? false : parent::move($from, $to); + } + }; + + $id = $this->id(); + $store->write($id, ['user' => 7]); + + $this->assertSame(7, $store->read($id)['data']['user']); + } + + public function test_destroy_still_invalidates_a_session_whose_file_cannot_be_deleted(): void + { + if (PHP_OS_FAMILY === 'Windows' || (function_exists('posix_geteuid') && posix_geteuid() === 0)) { + $this->markTestSkipped('Needs a POSIX user that cannot delete from a read-only directory.'); + } + + $store = new FileStore($this->path); + $id = $this->id(); + $store->write($id, ['user' => 7]); + + // Windows cannot delete a file another process has open. Removing write + // permission from the directory reproduces that: unlink() fails. + chmod($this->path . '/ab', 0500); + + try { + $store->destroy($id); + $this->assertNull($store->read($id), 'A destroyed session must not be readable.'); + } finally { + chmod($this->path . '/ab', 0700); + } + } + + public function test_a_filesystem_root_style_path_is_not_trimmed_away(): void + { + $store = new FileStore('/'); + $reflection = new \ReflectionProperty($store, 'path'); + + $this->assertSame('/', $reflection->getValue($store)); + } +} diff --git a/tests/Session/SessionManagerTest.php b/tests/Session/SessionManagerTest.php new file mode 100644 index 00000000..4dab5156 --- /dev/null +++ b/tests/Session/SessionManagerTest.php @@ -0,0 +1,35 @@ +assertSame($expected, $name); + + // PHP turns dots and spaces into underscores when it builds $_COOKIE, + // so the name must come back unchanged after that rewrite. + $this->assertSame($name, str_replace(['.', ' '], '_', $name)); + } + + public static function names(): array + { + return [ + 'plain' => ['doppar_session', 'doppar_session'], + 'space' => ['my app_session', 'my_app_session'], + 'dot' => ['my.app_session', 'my_app_session'], + 'hyphen kept' => ['my-app_session', 'my-app_session'], + 'unicode and separators' => ["caf\u{e9} \"x\";y", 'caf_x_y'], + 'empty falls back' => ['', 'doppar_session'], + 'only junk falls back' => [' ', 'doppar_session'], + ]; + } +} diff --git a/tests/Session/SessionTest.php b/tests/Session/SessionTest.php new file mode 100644 index 00000000..1713d4e1 --- /dev/null +++ b/tests/Session/SessionTest.php @@ -0,0 +1,390 @@ +path = sys_get_temp_dir() . '/doppar_session_' . bin2hex(random_bytes(6)); + $this->config = [ + 'driver' => 'file', + 'lifetime' => 120, + 'expire_on_close' => false, + 'cookie' => self::COOKIE, + 'path' => '/', + 'domain' => null, + 'secure' => false, + 'http_only' => true, + 'same_site' => 'lax', + 'stateless' => ['api/*'], + 'lottery' => [0, 100], + ]; + } + + protected function tearDown(): void + { + (new FileStore($this->path))->flush(); + + foreach (glob($this->path . '/*') ?: [] as $entry) { + is_dir($entry) ? @rmdir($entry) : @unlink($entry); + } + + @rmdir($this->path); + } + + private function store(): FileStore + { + return new FileStore($this->path, 7200); + } + + /** + * Build a request for the given path carrying the given cookies. + */ + private function request(string $path = '/', array $cookies = [], array $server = []): Request + { + $request = new class extends Request { + public string $fakePath = '/'; + + public function __construct() + { + // + } + + public function getPath(): string + { + return $this->fakePath; + } + }; + + $request->fakePath = $path; + $request->cookies = new InputBag($cookies); + $request->server = new ServerBag($server); + + return $request; + } + + private function session(?Request $request = null, ?FileStore $store = null): Session + { + $session = new Session($store ?? $this->store(), $this->config); + $session->attach($request ?? $this->request()); + + return $session; + } + + private function commit(Session $session): Response + { + $response = new Response(); + $session->commit($response); + + return $response; + } + + private function sessionCookie(Response $response): ?\Phaseolies\Http\Response\Cookie + { + foreach ($response->headers->getCookies() as $cookie) { + if ($cookie->getName() === self::COOKIE) { + return $cookie; + } + } + + return null; + } + + private function storedFiles(): array + { + return glob($this->path . '/*/*') ?: []; + } + + public function test_a_request_that_never_uses_the_session_stores_nothing_and_sends_no_cookie(): void + { + $session = $this->session(); + + $response = $this->commit($session); + + $this->assertNull($this->sessionCookie($response)); + $this->assertDirectoryDoesNotExist($this->path); + } + + public function test_reading_without_a_cookie_does_not_create_a_session(): void + { + $session = $this->session(); + + $this->assertNull($session->get('missing')); + $this->assertFalse($session->has('missing')); + $this->assertSame([], $session->all()); + + $this->assertNull($this->sessionCookie($this->commit($session))); + $this->assertSame([], $this->storedFiles()); + } + + public function test_writing_creates_one_session_and_sends_a_cookie(): void + { + $session = $this->session(); + $session->put('user', 7); + + $cookie = $this->sessionCookie($this->commit($session)); + + $this->assertNotNull($cookie); + $this->assertTrue($cookie->isHttpOnly()); + $this->assertSame('lax', $cookie->getSameSite()); + $this->assertCount(1, $this->storedFiles()); + + $next = $this->session($this->request('/', [self::COOKIE => $cookie->getValue()])); + $this->assertSame(7, $next->get('user')); + } + + public function test_a_second_request_with_the_cookie_reuses_the_same_session(): void + { + $first = $this->session(); + $first->put('a', 1); + $cookie = $this->sessionCookie($this->commit($first)); + + for ($i = 0; $i < 3; $i++) { + $session = $this->session($this->request('/', [self::COOKIE => $cookie->getValue()])); + $session->put('i', $i); + $this->commit($session); + } + + $this->assertCount(1, $this->storedFiles()); + } + + public function test_cookieless_requests_that_only_read_never_pile_up_files(): void + { + for ($i = 0; $i < 50; $i++) { + $session = $this->session(); + $session->get('anything'); + $this->commit($session); + } + + $this->assertSame([], $this->storedFiles()); + } + + public function test_csrf_token_is_created_lazily_and_is_stable(): void + { + $session = $this->session(); + $token = $session->token(); + $cookie = $this->sessionCookie($this->commit($session)); + + $this->assertSame(32, strlen($token)); + $this->assertSame($token, $session->token()); + + $next = $this->session($this->request('/', [self::COOKIE => $cookie->getValue()])); + $this->assertSame($token, $next->token()); + } + + public function test_stateless_paths_work_in_memory_but_store_nothing(): void + { + $session = $this->session($this->request('/api/users')); + $session->put('a', 1); + + $this->assertTrue($session->isStateless()); + $this->assertSame(1, $session->get('a')); + $this->assertNotEmpty($session->token()); + + $this->assertNull($this->sessionCookie($this->commit($session))); + $this->assertSame([], $this->storedFiles()); + } + + public function test_stateless_requests_ignore_an_incoming_session_cookie(): void + { + $first = $this->session(); + $first->put('secret', 'x'); + $cookie = $this->sessionCookie($this->commit($first)); + + $api = $this->session($this->request('/api/me', [self::COOKIE => $cookie->getValue()])); + + $this->assertNull($api->get('secret')); + } + + public function test_disable_turns_persistence_off_for_a_route(): void + { + $session = $this->session($this->request('/webhook')); + $session->disable(); + $session->put('a', 1); + + $this->assertNull($this->sessionCookie($this->commit($session))); + $this->assertSame([], $this->storedFiles()); + } + + public function test_regenerate_moves_the_data_to_a_new_id_and_removes_the_old_one(): void + { + $first = $this->session(); + $first->put('user', 7); + $old = $this->sessionCookie($this->commit($first))->getValue(); + + $session = $this->session($this->request('/', [self::COOKIE => $old])); + $session->regenerate(); + $new = $this->sessionCookie($this->commit($session))->getValue(); + + $this->assertNotSame($old, $new); + $this->assertCount(1, $this->storedFiles()); + $this->assertNull($this->store()->read($old)); + $this->assertSame(7, $this->store()->read($new)['data']['user']); + } + + public function test_invalidate_empties_the_session_and_expires_the_cookie(): void + { + $first = $this->session(); + $first->put('user', 7); + $old = $this->sessionCookie($this->commit($first))->getValue(); + + $session = $this->session($this->request('/', [self::COOKIE => $old])); + $session->invalidate(); + $cookie = $this->sessionCookie($this->commit($session)); + + $this->assertSame([], $this->storedFiles()); + $this->assertSame(1, $cookie->getExpiresTime()); + } + + public function test_destroy_removes_the_stored_session(): void + { + $first = $this->session(); + $first->put('user', 7); + $old = $this->sessionCookie($this->commit($first))->getValue(); + + $session = $this->session($this->request('/', [self::COOKIE => $old])); + $session->destroy(); + $this->commit($session); + + $this->assertNull($this->store()->read($old)); + $this->assertSame([], $session->all()); + } + + public function test_logout_style_invalidate_then_new_token_keeps_one_clean_session(): void + { + $first = $this->session(); + $first->put('user', 7); + $old = $this->sessionCookie($this->commit($first))->getValue(); + + $session = $this->session($this->request('/', [self::COOKIE => $old])); + $session->invalidate(); + $session->regenerateToken(); + $new = $this->sessionCookie($this->commit($session))->getValue(); + + $this->assertNotSame($old, $new); + $this->assertNull($this->store()->read($old)); + $this->assertArrayNotHasKey('user', $this->store()->read($new)['data']); + $this->assertCount(1, $this->storedFiles()); + } + + public function test_a_forged_or_unknown_cookie_is_treated_as_no_session(): void + { + foreach (['../../../etc/passwd', str_repeat('a', 40), 'x'] as $value) { + $session = $this->session($this->request('/', [self::COOKIE => $value])); + + $this->assertNull($session->get('user')); + $session->put('user', 1); + $cookie = $this->sessionCookie($this->commit($session)); + + $this->assertNotSame($value, $cookie->getValue()); + $this->assertMatchesRegularExpression(FileStore::ID_PATTERN, $cookie->getValue()); + } + } + + public function test_set_id_ignores_ids_that_are_not_well_formed(): void + { + $session = $this->session(); + $before = $session->getId(); + + $session->setId('../../x'); + + $this->assertSame($before, $session->getId()); + } + + public function test_an_old_session_id_is_rotated_but_keeps_its_data(): void + { + $id = bin2hex(random_bytes(20)); + $this->store()->write($id, ['user' => 7, 'last_regenerated' => time() - 99999]); + + $session = $this->session($this->request('/', [self::COOKIE => $id])); + $this->assertSame(7, $session->get('user')); + $new = $this->sessionCookie($this->commit($session))->getValue(); + + $this->assertNotSame($id, $new); + $this->assertNull($this->store()->read($id)); + $this->assertSame(7, $this->store()->read($new)['data']['user']); + } + + public function test_an_unchanged_session_is_not_rewritten(): void + { + $first = $this->session(); + $first->put('a', 1); + $value = $this->sessionCookie($this->commit($first))->getValue(); + $file = $this->path . '/' . substr($value, 0, 2) . '/' . $value; + if (PHP_OS_FAMILY === 'Windows') { + $this->markTestSkipped('Windows does not report stable inode numbers.'); + } + + $inode = fileinode($file); + + $session = $this->session($this->request('/', [self::COOKIE => $value])); + $session->get('a'); + $this->commit($session); + + clearstatcache(); + $this->assertSame($inode, fileinode($file)); + } + + public function test_peek_data_survives_a_redirect_like_flash(): void + { + $first = $this->session(); + $first->flash('status', 'saved'); + $value = $this->sessionCookie($this->commit($first))->getValue(); + + $next = $this->session($this->request('/', [self::COOKIE => $value])); + $this->assertSame('saved', $next->getPeek('status')); + $this->assertNull($next->get('status')); + } + + public function test_the_secure_flag_follows_the_request_when_not_configured(): void + { + $this->config['secure'] = null; + + foreach ([['HTTPS' => 'on', true], ['HTTPS' => 'off', false]] as $case) { + $expected = array_pop($case); + $session = $this->session($this->request('/', [], $case)); + $session->put('a', 1); + + $this->assertSame($expected, $this->sessionCookie($this->commit($session))->isSecure()); + } + } + + public function test_samesite_none_forces_a_secure_cookie(): void + { + $this->config['same_site'] = 'none'; + $session = $this->session(); + $session->put('a', 1); + + $this->assertTrue($this->sessionCookie($this->commit($session))->isSecure()); + } + + public function test_it_works_with_any_store_implementation(): void + { + $store = new ArrayStore(); + $session = new Session($store, $this->config); + $session->attach($this->request()); + $session->put('a', 1); + $cookie = $this->sessionCookie($this->commit($session)); + + $next = new Session($store, $this->config); + $next->attach($this->request('/', [self::COOKIE => $cookie->getValue()])); + + $this->assertSame(1, $next->get('a')); + } +} From 02f4166bc3cc5dee6d6bbe425833ba43a6bf460d Mon Sep 17 00:00:00 2001 From: techmahedy Date: Mon, 2 Nov 2026 16:43:29 +0600 Subject: [PATCH 2/6] fix: make session CI pass on PHPStan and Windows --- src/Phaseolies/Middleware/StatelessSession.php | 2 +- tests/Session/FileStoreTest.php | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Phaseolies/Middleware/StatelessSession.php b/src/Phaseolies/Middleware/StatelessSession.php index 3edcc9d2..d6f97cdd 100644 --- a/src/Phaseolies/Middleware/StatelessSession.php +++ b/src/Phaseolies/Middleware/StatelessSession.php @@ -14,7 +14,7 @@ class StatelessSession implements Middleware * Keep the session of this request in memory only * * @param Request $request - * @param \Closure(\Phaseolies\Http\Request) $next + * @param \Closure(\Phaseolies\Http\Request): \Phaseolies\Http\Response $next * @return \Phaseolies\Http\Response */ public function __invoke(Request $request, Closure $next): Response diff --git a/tests/Session/FileStoreTest.php b/tests/Session/FileStoreTest.php index 44cfe2c9..2f958a1f 100644 --- a/tests/Session/FileStoreTest.php +++ b/tests/Session/FileStoreTest.php @@ -174,8 +174,9 @@ public function test_writes_leave_no_temporary_files_behind(): void public function test_a_storage_path_containing_glob_characters_still_works(): void { - // [ ] { } * ? are legal in directory names on every OS and make glob() match nothing. - $path = $this->path . '/we[ir]d {dir} *?'; + // [ ] { } are legal in directory names on every OS and make glob() match nothing. + // * and ? are only added off Windows, where they are not valid in file names. + $path = $this->path . '/we[ir]d {dir}' . (DIRECTORY_SEPARATOR === '\\' ? '' : ' *?'); $store = new FileStore($path, 60); $fresh = $this->id('aa'); From 86817d8f7d8dbd29afa79aa1c17c744967f31431 Mon Sep 17 00:00:00 2001 From: techmahedy Date: Mon, 2 Nov 2026 19:21:10 +0600 Subject: [PATCH 3/6] fix: address session review (merge concurrent writes, secure logout cookie, lazy CSRF cookie) --- src/Phaseolies/Application.php | 27 +++- .../Middleware/CsrfTokenMiddleware.php | 7 + .../Session/Contracts/CommittableSession.php | 7 + .../Contracts/LockableSessionStore.php | 15 ++ src/Phaseolies/Session/Stores/CookieStore.php | 32 ++++- src/Phaseolies/Session/Stores/FileStore.php | 48 ++++++- src/Phaseolies/Support/Session.php | 135 ++++++++++++++++-- tests/Session/CookieStoreTest.php | 44 ++++++ tests/Session/DispatchSessionTest.php | 33 +++++ tests/Session/SessionTest.php | 122 ++++++++++++++++ 10 files changed, 448 insertions(+), 22 deletions(-) create mode 100644 src/Phaseolies/Session/Contracts/LockableSessionStore.php diff --git a/src/Phaseolies/Application.php b/src/Phaseolies/Application.php index eab904e1..f96d0dd5 100644 --- a/src/Phaseolies/Application.php +++ b/src/Phaseolies/Application.php @@ -1212,7 +1212,7 @@ public function defer(callable $callback): self if (!$scope instanceof DeferScope) { throw new \LogicException( 'defer() was called outside an active execution scope. ' - . 'Run the code through runInDeferScope(), an HTTP dispatch or a console command.' + . 'Run the code through runInDeferScope(), an HTTP dispatch or a console command.' ); } @@ -1553,13 +1553,18 @@ public function dispatch($request): DispatchResult $response->prepare($request)->send(); + $this->terminateSession($request); + $this->suspendDeferScope($deferScope); $this->suspendContext($context); return new DispatchResult($this, $request, $response, null, $deferScope, $context); } catch (HttpException $e) { try { - $this->commitSession($request); + try { + $this->commitSession($request); + } catch (\Throwable) { + } Response::dispatchHttpException($e); } catch (\Throwable $renderFailure) { @@ -1609,6 +1614,24 @@ protected function commitSession(Request $request, ?Response $response = null): } } + /** + * Run the session housekeeping once the response has been sent + * + * @param Request $request + * @return void + */ + protected function terminateSession(Request $request): void + { + $session = $request->session(); + + if ($session instanceof CommittableSession) { + try { + $session->terminate(); + } catch (\Throwable) { + } + } + } + /** * Describe a request for its execution context. Must never throw. * diff --git a/src/Phaseolies/Middleware/CsrfTokenMiddleware.php b/src/Phaseolies/Middleware/CsrfTokenMiddleware.php index 447f0204..e76daf76 100644 --- a/src/Phaseolies/Middleware/CsrfTokenMiddleware.php +++ b/src/Phaseolies/Middleware/CsrfTokenMiddleware.php @@ -112,6 +112,13 @@ protected function addCookieToResponse($request, $response): Response return $response; } + // Do not create the token just to hand it out: that would make every + // anonymous visit worth a stored session. A page that renders a form + // or reads csrf_token() has created it by now. + if (!$session->has('_token')) { + return $response; + } + $config = config('session'); $response->headers->setCookie($this->addCookie($request, $config)); diff --git a/src/Phaseolies/Session/Contracts/CommittableSession.php b/src/Phaseolies/Session/Contracts/CommittableSession.php index df619249..8ab27090 100644 --- a/src/Phaseolies/Session/Contracts/CommittableSession.php +++ b/src/Phaseolies/Session/Contracts/CommittableSession.php @@ -43,4 +43,11 @@ public function isStateless(): bool; * @return bool */ public function isLoaded(): bool; + + /** + * Run the housekeeping that must not delay the response + * + * @return void + */ + public function terminate(): void; } diff --git a/src/Phaseolies/Session/Contracts/LockableSessionStore.php b/src/Phaseolies/Session/Contracts/LockableSessionStore.php new file mode 100644 index 00000000..da6a113a --- /dev/null +++ b/src/Phaseolies/Session/Contracts/LockableSessionStore.php @@ -0,0 +1,15 @@ + $id, 'exp' => time() + $this->lifetime, 'data' => $data]; - if (json_encode($payload) === false) { - throw new SessionException('Session data cannot be stored in a cookie because it is not JSON serializable.'); - } + $this->assertJsonSafe($data); $value = $this->encrypter->encrypt($payload); - if (strlen($value) > self::MAX_COOKIE_BYTES) { + // Browsers count the cookie as it is sent, after percent-encoding. + $size = strlen(rawurlencode($value)); + + if ($size > self::MAX_COOKIE_BYTES) { throw new SessionException(sprintf( 'Session data is too large for the cookie session driver (%d of %d bytes). Largest keys: %s. Store less in the session or use the file driver.', - strlen($value), + $size, self::MAX_COOKIE_BYTES, $this->largestKeys($data) )); @@ -94,6 +95,27 @@ public function write(string $id, array $data): string return $value; } + /** + * Make sure the data comes back from the cookie exactly as it went in + * + * @param array $data + * @return void + * @throws SessionException + */ + protected function assertJsonSafe(array $data): void + { + foreach ($data as $key => $value) { + $json = json_encode($value); + + if ($json === false || json_decode($json, true) !== $value) { + throw new SessionException(sprintf( + 'Session key "%s" cannot be stored by the cookie session driver. It must contain only strings (valid UTF-8), numbers, booleans, null and arrays, not objects or binary data. Convert it first or use the file driver.', + $key + )); + } + } + } + /** * Describe the keys that take up the most room, by name and size only. * diff --git a/src/Phaseolies/Session/Stores/FileStore.php b/src/Phaseolies/Session/Stores/FileStore.php index c0a6839d..c89d8270 100644 --- a/src/Phaseolies/Session/Stores/FileStore.php +++ b/src/Phaseolies/Session/Stores/FileStore.php @@ -2,10 +2,10 @@ namespace Phaseolies\Session\Stores; -use Phaseolies\Session\Contracts\SessionStoreInterface; +use Phaseolies\Session\Contracts\LockableSessionStore; use Phaseolies\Session\Exceptions\SessionException; -class FileStore implements SessionStoreInterface +class FileStore implements LockableSessionStore { /** * Pattern every generated session id matches. @@ -43,7 +43,8 @@ public function read(string $cookieValue): ?array // No lock is taken: writes replace the file atomically, so a reader // sees either the old or the new session. File locks are mandatory on - // Windows and would only make readers and writers collide. + // Windows and would only make readers and writers collide. Concurrent + // writers are merged under synchronized() instead. // PHP caches stat results; long-lived workers must not see stale times. clearstatcache(true, $file); @@ -114,6 +115,47 @@ public function write(string $id, array $data): string return $id; } + /** + * Run a callback while no other request can save a session of the same shard + * + * @param string $id + * @param callable $callback + * @return mixed + */ + public function synchronized(string $id, callable $callback): mixed + { + $directory = $this->path . '/.locks'; + + if (!self::isValidId($id) || (!is_dir($directory) && !@mkdir($directory, 0700, true) && !is_dir($directory))) { + return $callback(); + } + + $handle = @fopen($directory . '/' . substr($id, 0, 2), 'c'); + + if ($handle === false) { + return $callback(); + } + + $deadline = microtime(true) + 2; + + while (!flock($handle, LOCK_EX | LOCK_NB)) { + if (microtime(true) > $deadline) { + fclose($handle); + + return $callback(); + } + + usleep(5000); + } + + try { + return $callback(); + } finally { + flock($handle, LOCK_UN); + fclose($handle); + } + } + /** * Refresh the lifetime of a session without rewriting its data * diff --git a/src/Phaseolies/Support/Session.php b/src/Phaseolies/Support/Session.php index 60f58b9d..357a0cf0 100644 --- a/src/Phaseolies/Support/Session.php +++ b/src/Phaseolies/Support/Session.php @@ -6,6 +6,7 @@ use Phaseolies\Http\Response; use Phaseolies\Http\Response\Cookie; use Phaseolies\Session\Contracts\CommittableSession; +use Phaseolies\Session\Contracts\LockableSessionStore; use Phaseolies\Session\Contracts\SessionInterface; use Phaseolies\Session\Contracts\SessionStoreInterface; use Phaseolies\Session\SessionManager; @@ -71,6 +72,21 @@ class Session implements SessionInterface, CommittableSession */ protected ?string $snapshot = null; + /** + * The data as it was loaded, to find what this request changed. + */ + protected array $original = []; + + /** + * Whether the stored data must be replaced instead of merged on commit. + */ + protected bool $replace = false; + + /** + * Whether the session was saved by this request. + */ + protected bool $committed = false; + /** * Whether the cookie must be expired when the session is committed. */ @@ -191,10 +207,23 @@ public function commit(?Response $response = null): void $this->id ??= self::newId(); $this->data['last_regenerated'] ??= time(); - $changed = $this->storedId !== $this->id || serialize($this->data) !== $this->snapshot; + $serialized = serialize($this->data); + $changed = $this->storedId !== $this->id || $serialized !== $this->snapshot; if ($changed || $store->isClientSide()) { - $value = $store->write($this->id, $this->data); + if ($store instanceof LockableSessionStore && $this->storedId !== null) { + // Another request may have saved this session since it was + // loaded. Apply only what this request changed on top of it. + $value = $store->synchronized($this->storedId, function () use ($store): string { + $this->mergeConcurrentChanges($store); + + return $store->write($this->id, $this->data); + }); + + $serialized = serialize($this->data); + } else { + $value = $store->write($this->id, $this->data); + } } else { $store->touch($this->id); $value = $this->id; @@ -206,11 +235,82 @@ public function commit(?Response $response = null): void $this->storedId = $this->id; $this->previousId = null; - $this->snapshot = serialize($this->data); + $this->snapshot = $serialized; + $this->original = $this->data; + $this->replace = false; + $this->committed = true; $this->queueCookie($response, $this->makeCookie($value, $config)); + } + + /** + * Run the housekeeping that must not delay the response. + * + * @return void + */ + public function terminate(): void + { + if ($this->committed) { + $this->committed = false; - $this->sweep($store, $config); + $this->sweep($this->store(), $this->config()); + } + } + + /** + * Apply this request's changes on top of the session as currently stored. + * + * @param SessionStoreInterface $store + * @return void + */ + protected function mergeConcurrentChanges(SessionStoreInterface $store): void + { + if ($this->replace) { + return; + } + + $record = $store->read($this->storedId); + + if ($record === null || serialize($record['data']) === $this->snapshot) { + return; + } + + $this->data = $this->mergeChanges($record['data'], $this->original, $this->data); + } + + /** + * Copy what changed between $original and $current onto $stored. + * + * @param array $stored + * @param array $original + * @param array $current + * @return array + */ + protected function mergeChanges(array $stored, array $original, array $current): array + { + foreach ($original as $key => $value) { + if (!array_key_exists($key, $current)) { + unset($stored[$key]); + } + } + + foreach ($current as $key => $value) { + $known = array_key_exists($key, $original); + + if ($known && $original[$key] === $value) { + continue; + } + + if ($known && is_array($value) && is_array($original[$key]) && isset($stored[$key]) && is_array($stored[$key])) { + $stored[$key] = $this->mergeChanges($stored[$key], $original[$key], $value); + + continue; + } + + $stored[$key] = $value; + } + + return $stored; } /** @@ -232,7 +332,7 @@ protected function load(): void if ($record !== null) { $this->id = $this->storedId = $record['id']; - $this->data = $record['data']; + $this->data = $this->original = $record['data']; $this->snapshot = serialize($this->data); if ($this->shouldRotate()) { @@ -315,16 +415,20 @@ protected function makeCookie(string $value, array $config): Cookie */ protected function expiredCookie(array $config): Cookie { + $cookie = $this->makeCookie('', $config); + + // Same attributes as the cookie being removed, or browsers ignore it: + // Chrome rejects SameSite=None without Secure, deletions included. return new Cookie( - $config['cookie'], + $cookie->getName(), '', 1, - $config['path'], - $config['domain'], - (bool) ($config['secure'] ?? false), - (bool) $config['http_only'], + $cookie->getPath(), + $cookie->getDomain(), + $cookie->isSecure(), + $cookie->isHttpOnly(), false, - strtolower((string) ($config['same_site'] ?: 'lax')) + $cookie->getSameSite() ); } @@ -512,6 +616,7 @@ public function flush(): void $this->load(); $this->data = []; + $this->replace = true; } /** @@ -572,8 +677,13 @@ public function setId(string $id): void $store = $this->store(); + // Switching ids must not leave the old session valid. + if ($this->storedId !== null && $this->storedId !== $id) { + $this->previousId ??= $this->storedId; + } + if (!$store->isClientSide() && !$this->isStateless() && ($record = $store->read($id)) !== null) { - $this->data = $record['data']; + $this->data = $this->original = $record['data']; $this->storedId = $record['id']; $this->snapshot = serialize($this->data); } @@ -592,6 +702,7 @@ public function destroy(): void $this->previousId ??= $this->storedId; $this->storedId = $this->id = $this->snapshot = null; + $this->original = []; $this->forgetCookie = true; $this->data = []; $this->peek = []; diff --git a/tests/Session/CookieStoreTest.php b/tests/Session/CookieStoreTest.php index 9a5a5d51..6de8c337 100644 --- a/tests/Session/CookieStoreTest.php +++ b/tests/Session/CookieStoreTest.php @@ -64,4 +64,48 @@ public function test_it_is_client_side(): void { $this->assertTrue((new CookieStore(new Encryption()))->isClientSide()); } + + public function test_it_refuses_objects_because_they_would_come_back_as_arrays(): void + { + try { + (new CookieStore(new Encryption()))->write($this->id(), ['ok' => 1, 'user' => (object) ['id' => 1]]); + $this->fail('Expected an exception.'); + } catch (SessionException $e) { + $this->assertStringContainsString('"user"', $e->getMessage()); + } + } + + public function test_it_refuses_binary_data_and_names_the_key(): void + { + try { + (new CookieStore(new Encryption()))->write($this->id(), ['blob' => "\xff\xfe\x00binary"]); + $this->fail('Expected an exception.'); + } catch (SessionException $e) { + $this->assertStringContainsString('"blob"', $e->getMessage()); + } + } + + public function test_it_accepts_values_that_survive_json_unchanged(): void + { + $store = new CookieStore(new Encryption()); + $data = ['s' => 'é', 'i' => 1, 'f' => 1.5, 'b' => true, 'n' => null, 'a' => ['x' => [1, 2]]]; + $id = $this->id(); + + $this->assertSame($data, $store->read($store->write($id, $data))['data']); + } + + public function test_the_size_limit_is_measured_after_percent_encoding(): void + { + // 2,000 raw bytes, but every "+" becomes "%2B" in the Set-Cookie header. + $encrypter = new class extends Encryption { + public function encrypt(mixed $data): string + { + return str_repeat('+', 2000); + } + }; + + $this->expectException(SessionException::class); + + (new CookieStore($encrypter))->write($this->id(), ['a' => 1]); + } } diff --git a/tests/Session/DispatchSessionTest.php b/tests/Session/DispatchSessionTest.php index 001537c2..aecc73e9 100644 --- a/tests/Session/DispatchSessionTest.php +++ b/tests/Session/DispatchSessionTest.php @@ -144,4 +144,37 @@ public function write(string $id, array $data): string $this->app->dispatch($request); } + + public function testAFailingSessionSaveDoesNotTurnAnHttpExceptionIntoAnError(): void + { + $request = $this->request(); + $request->session = new Session(new class extends ArrayStore { + public function write(string $id, array $data): string + { + throw new RuntimeException('store down'); + } + }, ['cookie' => self::COOKIE, 'lifetime' => 120, 'stateless' => [], 'lottery' => [0, 100]]); + $request->session->attach($request); + + $this->routeTo(function () use ($request) { + $request->session()->put('a', 1); + + throw new HttpException(404, 'Not found'); + }); + + // The error page is rendered for real; keep it out of the test output. + ob_start(); + set_error_handler(static fn() => true); + + try { + $this->app->dispatch($request); + } catch (RuntimeException $e) { + $this->assertNotSame('store down', $e->getMessage()); + } finally { + restore_error_handler(); + ob_end_clean(); + } + + $this->addToAssertionCount(1); + } } diff --git a/tests/Session/SessionTest.php b/tests/Session/SessionTest.php index 1713d4e1..01923eae 100644 --- a/tests/Session/SessionTest.php +++ b/tests/Session/SessionTest.php @@ -387,4 +387,126 @@ public function test_it_works_with_any_store_implementation(): void $this->assertSame(1, $next->get('a')); } + + public function test_the_deletion_cookie_is_secure_too_when_samesite_is_none(): void + { + $this->config['same_site'] = 'none'; + $this->config['secure'] = false; + $session = $this->session(); + $session->put('a', 1); + $cookie = $this->sessionCookie($this->commit($session)); + + $next = $this->session($this->request('/', [self::COOKIE => $cookie->getValue()])); + $next->invalidate(); + $expired = $this->sessionCookie($this->commit($next)); + + $this->assertLessThan(time(), $expired->getExpiresTime()); + $this->assertTrue($expired->isSecure()); + $this->assertSame('none', strtolower((string) $expired->getSameSite())); + } + + private function twoRequestsOnOneSession(): array + { + $first = $this->session(); + $first->put('user', 1); + $first->put('_flash', ['kept' => 'yes']); + $cookie = $this->sessionCookie($this->commit($first))->getValue(); + + $make = fn() => $this->session($this->request('/', [self::COOKIE => $cookie])); + $a = $make(); + $b = $make(); + $a->get('user'); + $b->get('user'); + + return [$cookie, $a, $b]; + } + + public function test_concurrent_requests_do_not_overwrite_each_others_changes(): void + { + [$cookie, $a, $b] = $this->twoRequestsOnOneSession(); + + $a->put('cart', ['apple']); + $a->put('_flash', ['kept' => 'yes', 'from_a' => 'a']); + $b->put('wishlist', ['pear']); + $b->put('_flash', ['kept' => 'yes', 'from_b' => 'b']); + $b->forget('user'); + + $this->commit($a); + $this->commit($b); + + $merged = $this->session($this->request('/', [self::COOKIE => $cookie]))->all(); + + $this->assertSame(['apple'], $merged['cart']); + $this->assertSame(['pear'], $merged['wishlist']); + $this->assertArrayNotHasKey('user', $merged); + $this->assertSame(['kept' => 'yes', 'from_a' => 'a', 'from_b' => 'b'], $merged['_flash']); + } + + public function test_the_last_writer_wins_for_the_same_key(): void + { + [$cookie, $a, $b] = $this->twoRequestsOnOneSession(); + + $a->put('user', 2); + $b->put('user', 3); + $this->commit($a); + $this->commit($b); + + $this->assertSame(3, $this->session($this->request('/', [self::COOKIE => $cookie]))->get('user')); + } + + public function test_an_invalidated_session_is_not_merged_with_concurrent_changes(): void + { + [$cookie, $a, $b] = $this->twoRequestsOnOneSession(); + + $a->put('cart', ['apple']); + $this->commit($a); + + $b->invalidate(); + $this->commit($b); + + $this->assertNull($this->session($this->request('/', [self::COOKIE => $cookie]))->get('cart')); + $this->assertSame([], $this->storedFiles()); + } + + public function test_set_id_removes_the_session_it_replaces(): void + { + $session = $this->session(); + $session->put('a', 1); + $old = $this->sessionCookie($this->commit($session))->getValue(); + + $next = $this->session($this->request('/', [self::COOKIE => $old])); + $new = bin2hex(random_bytes(20)); + $next->setId($new); + $next->put('b', 2); + $this->commit($next); + + $this->assertNull($this->store()->read($old)); + $this->assertNotNull($this->store()->read($new)); + } + + public function test_the_sweep_runs_after_the_response_not_while_saving(): void + { + $this->config['lottery'] = [1, 1]; + $store = new class($this->path, 7200) extends FileStore { + public int $sweeps = 0; + + public function sweep(int $maxLifetime): int + { + $this->sweeps++; + + return 0; + } + }; + + $session = $this->session(null, $store); + $session->put('a', 1); + $this->commit($session); + + $this->assertSame(0, $store->sweeps); + + $session->terminate(); + $session->terminate(); + + $this->assertSame(1, $store->sweeps); + } } From 3cfbfdfb98569b63f1fe1659df9810d5e34ca349 Mon Sep 17 00:00:00 2001 From: techmahedy Date: Tue, 3 Nov 2026 11:00:48 +0600 Subject: [PATCH 4/6] fix: keep concurrent list appends, stop late requests resurrecting a logged-out session --- .../Middleware/CsrfTokenMiddleware.php | 40 ++- .../Contracts/LockableSessionStore.php | 2 +- src/Phaseolies/Session/SessionManager.php | 1 + src/Phaseolies/Session/Stores/FileStore.php | 62 ++++- src/Phaseolies/Support/Session.php | 155 +++++++++++- tests/Session/CsrfCookieTest.php | 84 +++++++ tests/Session/DispatchSessionTest.php | 5 +- tests/Session/FileStoreTest.php | 126 ++++++++++ tests/Session/SessionTest.php | 229 ++++++++++++++++++ 9 files changed, 667 insertions(+), 37 deletions(-) create mode 100644 tests/Session/CsrfCookieTest.php diff --git a/src/Phaseolies/Middleware/CsrfTokenMiddleware.php b/src/Phaseolies/Middleware/CsrfTokenMiddleware.php index e76daf76..e2d99b22 100644 --- a/src/Phaseolies/Middleware/CsrfTokenMiddleware.php +++ b/src/Phaseolies/Middleware/CsrfTokenMiddleware.php @@ -105,25 +105,45 @@ protected function addCookieToResponse($request, $response): Response return $response; } + if (!$this->shouldIssueCookie($request)) { + return $response; + } + + $response->headers->setCookie($this->addCookie($request, config('session'))); + + return $response; + } + + /** + * Decide whether this response should carry the XSRF-TOKEN cookie + * + * @param \Phaseolies\Http\Request $request + * @return bool + */ + protected function shouldIssueCookie($request): bool + { $session = $request->session(); // Stateless requests have no stored session to tie the token to. if ($session instanceof CommittableSession && $session->isStateless()) { - return $response; + return false; } - // Do not create the token just to hand it out: that would make every - // anonymous visit worth a stored session. A page that renders a form - // or reads csrf_token() has created it by now. - if (!$session->has('_token')) { - return $response; + if ($session->has('_token')) { + return true; } - $config = config('session'); - - $response->headers->setCookie($this->addCookie($request, $config)); + return $this->alwaysSendsCookie(); + } - return $response; + /** + * Whether the cookie is sent even before a token exists. + * + * @return bool + */ + protected function alwaysSendsCookie(): bool + { + return (bool) (((array) config('session'))['always_send_xsrf_cookie'] ?? false); } /** diff --git a/src/Phaseolies/Session/Contracts/LockableSessionStore.php b/src/Phaseolies/Session/Contracts/LockableSessionStore.php index da6a113a..12ae00a2 100644 --- a/src/Phaseolies/Session/Contracts/LockableSessionStore.php +++ b/src/Phaseolies/Session/Contracts/LockableSessionStore.php @@ -5,7 +5,7 @@ interface LockableSessionStore extends SessionStoreInterface { /** - * Run a callback while no other request can save the same session + * Run a callback while no other request can save or destroy the same session * * @param string $id * @param callable $callback diff --git a/src/Phaseolies/Session/SessionManager.php b/src/Phaseolies/Session/SessionManager.php index cbf4110a..93b33243 100644 --- a/src/Phaseolies/Session/SessionManager.php +++ b/src/Phaseolies/Session/SessionManager.php @@ -30,6 +30,7 @@ class SessionManager 'same_site' => 'lax', 'stateless' => [], 'lottery' => [2, 100], + 'always_send_xsrf_cookie' => false, ]; /** diff --git a/src/Phaseolies/Session/Stores/FileStore.php b/src/Phaseolies/Session/Stores/FileStore.php index c89d8270..90ca6d7e 100644 --- a/src/Phaseolies/Session/Stores/FileStore.php +++ b/src/Phaseolies/Session/Stores/FileStore.php @@ -18,9 +18,13 @@ class FileStore implements LockableSessionStore /** * @param string $path * @param int $lifetime + * @param float $lockTimeout */ - public function __construct(protected string $path, protected int $lifetime = 7200) - { + public function __construct( + protected string $path, + protected int $lifetime = 7200, + protected float $lockTimeout = 2.0 + ) { $trimmed = rtrim($path, '/\\'); // Keep a filesystem root ("/" or "C:\") intact instead of trimming it away. @@ -116,7 +120,7 @@ public function write(string $id, array $data): string } /** - * Run a callback while no other request can save a session of the same shard + * Run a callback while no other request can save or destroy a session of the same shard * * @param string $id * @param callable $callback @@ -126,22 +130,35 @@ public function synchronized(string $id, callable $callback): mixed { $directory = $this->path . '/.locks'; - if (!self::isValidId($id) || (!is_dir($directory) && !@mkdir($directory, 0700, true) && !is_dir($directory))) { + if (!self::isValidId($id)) { + return $callback(); + } + + if (!is_dir($directory) && !@mkdir($directory, 0700, true) && !is_dir($directory)) { + $this->report('Session locking is unavailable: the lock directory could not be created. Saving without a lock.'); + return $callback(); } $handle = @fopen($directory . '/' . substr($id, 0, 2), 'c'); if ($handle === false) { + $this->report('Session locking is unavailable: the lock file could not be opened. Saving without a lock.'); + return $callback(); } - $deadline = microtime(true) + 2; + $deadline = microtime(true) + $this->lockTimeout; while (!flock($handle, LOCK_EX | LOCK_NB)) { if (microtime(true) > $deadline) { fclose($handle); + $this->report(sprintf( + 'Session lock was not acquired within %s seconds. Saving without a lock, so concurrent changes to this session may be lost.', + rtrim(rtrim(number_format($this->lockTimeout, 3, '.', ''), '0'), '.') + )); + return $callback(); } @@ -156,6 +173,23 @@ public function synchronized(string $id, callable $callback): mixed } } + /** + * Report a condition that does not stop the request but should not go unnoticed. + * + * @param string $message + * @return void + */ + protected function report(string $message): void + { + try { + if (function_exists('warning')) { + warning('[Doppar] ' . $message); + } + } catch (\Throwable) { + // Logging must never break a request, least of all while it is saving. + } + } + /** * Refresh the lifetime of a session without rewriting its data * @@ -192,16 +226,18 @@ public function destroy(string $id): void return; } - $file = $this->file($id); + $this->synchronized($id, function () use ($id): void { + $file = $this->file($id); - if (@unlink($file) || !file_exists($file)) { - return; - } + if (@unlink($file) || !file_exists($file)) { + return; + } - // Windows refuses to delete a file another process has open. The - // session must still stop working, so empty it: an empty file reads - // as an unknown session and gc() removes it later. - @file_put_contents($file, ''); + // Windows refuses to delete a file another process has open. The + // session must still stop working, so empty it: an empty file reads + // as an unknown session and gc() removes it later. + @file_put_contents($file, ''); + }); } /** diff --git a/src/Phaseolies/Support/Session.php b/src/Phaseolies/Support/Session.php index 357a0cf0..8d77a220 100644 --- a/src/Phaseolies/Support/Session.php +++ b/src/Phaseolies/Support/Session.php @@ -214,12 +214,23 @@ public function commit(?Response $response = null): void if ($store instanceof LockableSessionStore && $this->storedId !== null) { // Another request may have saved this session since it was // loaded. Apply only what this request changed on top of it. - $value = $store->synchronized($this->storedId, function () use ($store): string { - $this->mergeConcurrentChanges($store); + $value = $store->synchronized($this->storedId, function () use ($store): ?string { + if (!$this->mergeConcurrentChanges($store)) { + return null; + } return $store->write($this->id, $this->data); }); + if ($value === null) { + // Logged out, destroyed or rotated by another request while + // this one was running. Writing it back would undo that, and + // sending its cookie would replace the one that request issued. + $this->abandon(); + + return; + } + $serialized = serialize($this->data); } else { $value = $store->write($this->id, $this->data); @@ -258,24 +269,42 @@ public function terminate(): void } /** - * Apply this request's changes on top of the session as currently stored. + * Forget that this session is stored, without touching the store or the cookie. * - * @param SessionStoreInterface $store * @return void */ - protected function mergeConcurrentChanges(SessionStoreInterface $store): void + protected function abandon(): void { - if ($this->replace) { - return; - } + $this->storedId = $this->previousId = $this->snapshot = null; + $this->original = []; + $this->replace = $this->committed = false; + } + /** + * Apply this request's changes on top of the session as currently stored. + * + * @param SessionStoreInterface $store + * @return bool + */ + protected function mergeConcurrentChanges(SessionStoreInterface $store): bool + { $record = $store->read($this->storedId); - if ($record === null || serialize($record['data']) === $this->snapshot) { - return; + if ($record === null) { + // The record this request loaded no longer exists: another request + // logged out, destroyed or expired it. Saving under the same id + // would resurrect it, while a new id (regenerate, setId) is a new + // session and is simply created. + return $this->id !== $this->storedId; + } + + if ($this->replace || serialize($record['data']) === $this->snapshot) { + return true; } $this->data = $this->mergeChanges($record['data'], $this->original, $this->data); + + return true; } /** @@ -301,8 +330,10 @@ protected function mergeChanges(array $stored, array $original, array $current): continue; } - if ($known && is_array($value) && is_array($original[$key]) && isset($stored[$key]) && is_array($stored[$key])) { - $stored[$key] = $this->mergeChanges($stored[$key], $original[$key], $value); + $base = $known ? $original[$key] : []; + + if (is_array($value) && is_array($base) && isset($stored[$key]) && is_array($stored[$key])) { + $stored[$key] = $this->mergeValues($stored[$key], $base, $value); continue; } @@ -313,6 +344,106 @@ protected function mergeChanges(array $stored, array $original, array $current): return $stored; } + /** + * Merge two versions of an array value. + * + * @param array $stored + * @param array $original + * @param array $current + * @return array + */ + protected function mergeValues(array $stored, array $original, array $current): array + { + if (array_is_list($stored) && array_is_list($original) && array_is_list($current)) { + return $this->mergeLists($stored, $original, $current); + } + + return $this->mergeChanges($stored, $original, $current); + } + + /** + * Merge two versions of a list by what each request added and removed. + * + * @param array $stored + * @param array $original + * @param array $current + * @return array + */ + protected function mergeLists(array $stored, array $original, array $current): array + { + if ($this->sameList($stored, $original)) { + // Nobody else touched the list, so this request's version stands. + return $current; + } + + $removed = $this->missingFrom($original, $current); + $added = $this->missingFrom($current, $original); + + foreach ($removed as $item) { + foreach ($stored as $index => $candidate) { + if ($this->sameItem($candidate, $item)) { + unset($stored[$index]); + + break; + } + } + } + + return array_values(array_merge($stored, $added)); + } + + /** + * The items of $items that have no counterpart in $other, counting duplicates. + * + * @param array $items + * @param array $other + * @return array + */ + protected function missingFrom(array $items, array $other): array + { + $missing = []; + + foreach ($items as $item) { + foreach ($other as $index => $candidate) { + if ($this->sameItem($item, $candidate)) { + unset($other[$index]); + + continue 2; + } + } + + $missing[] = $item; + } + + return $missing; + } + + /** + * @param array $first + * @param array $second + * @return bool + */ + protected function sameList(array $first, array $second): bool + { + return serialize($first) === serialize($second); + } + + /** + * Whether two list items are identical. + * + * @param mixed $first + * @param mixed $second + * @return bool + */ + protected function sameItem(mixed $first, mixed $second): bool + { + if (is_scalar($first) || $first === null || is_scalar($second) || $second === null) { + return $first === $second; + } + + return serialize($first) === serialize($second); + } + /** * Look up the stored session the first time the session is used. * diff --git a/tests/Session/CsrfCookieTest.php b/tests/Session/CsrfCookieTest.php new file mode 100644 index 00000000..6928e84c --- /dev/null +++ b/tests/Session/CsrfCookieTest.php @@ -0,0 +1,84 @@ + 'test_session', + 'lifetime' => 120, + 'stateless' => [], + 'lottery' => [0, 100], + ]); + } + + private function request(Session $session): Request + { + $request = new Request(); + $request->session = $session; + $session->attach($request); + + return $request; + } + + private function decide(Request $request, bool $always = false): bool + { + $middleware = new class($always) extends CsrfTokenMiddleware { + public function __construct(private bool $always) + { + } + + protected function alwaysSendsCookie(): bool + { + return $this->always; + } + + public function decide($request): bool + { + return $this->shouldIssueCookie($request); + } + }; + + return $middleware->decide($request); + } + + public function test_no_cookie_is_sent_before_a_token_exists(): void + { + $session = $this->session(); + + $this->assertFalse($this->decide($this->request($session))); + $this->assertFalse($session->has('_token'), 'Deciding must not create the token.'); + } + + public function test_the_cookie_is_sent_once_a_token_exists(): void + { + $session = $this->session(); + $session->token(); + + $this->assertTrue($this->decide($this->request($session))); + } + + public function test_the_always_send_option_sends_the_cookie_on_the_first_response(): void + { + $session = $this->session(); + + $this->assertTrue($this->decide($this->request($session), always: true)); + } + + public function test_stateless_requests_never_get_the_cookie_even_with_the_option_on(): void + { + $session = $this->session(['stateless' => ['api/*']]); + $request = $this->request($session); + $session->disable(); + + $this->assertFalse($this->decide($request, always: true)); + } +} diff --git a/tests/Session/DispatchSessionTest.php b/tests/Session/DispatchSessionTest.php index aecc73e9..0d03321f 100644 --- a/tests/Session/DispatchSessionTest.php +++ b/tests/Session/DispatchSessionTest.php @@ -168,7 +168,10 @@ public function write(string $id, array $data): string try { $this->app->dispatch($request); - } catch (RuntimeException $e) { + } catch (\Throwable $e) { + // Rendering the error page needs a bootstrapped application, so it may + // raise something here. Whatever it raises, it must not be the failure + // of the session store, which would have replaced the original error. $this->assertNotSame('store down', $e->getMessage()); } finally { restore_error_handler(); diff --git a/tests/Session/FileStoreTest.php b/tests/Session/FileStoreTest.php index 2f958a1f..6efc26c4 100644 --- a/tests/Session/FileStoreTest.php +++ b/tests/Session/FileStoreTest.php @@ -271,4 +271,130 @@ public function test_a_filesystem_root_style_path_is_not_trimmed_away(): void $this->assertSame('/', $reflection->getValue($store)); } + + private function loggingStore(float $timeout = 0.05): FileStore + { + return new class($this->path, 7200, $timeout) extends FileStore { + /** @var string[] */ + public array $reports = []; + + protected function report(string $message): void + { + $this->reports[] = $message; + } + }; + } + + /** + * Take the lock of the shard an id belongs to from outside the store. + * + * @return resource + */ + private function holdLockOf(string $id) + { + $directory = $this->path . '/.locks'; + is_dir($directory) || mkdir($directory, 0700, true); + + $handle = fopen($directory . '/' . substr($id, 0, 2), 'c'); + $this->assertTrue(flock($handle, LOCK_EX | LOCK_NB), 'The test could not take the lock.'); + + return $handle; + } + + public function test_giving_up_on_a_session_lock_is_logged_and_the_callback_still_runs(): void + { + $store = $this->loggingStore(0.05); + $id = $this->id(); + $held = $this->holdLockOf($id); + + $ran = $store->synchronized($id, fn () => 'ran'); + + flock($held, LOCK_UN); + fclose($held); + + $this->assertSame('ran', $ran); + $this->assertCount(1, $store->reports); + $this->assertStringContainsString('not acquired', $store->reports[0]); + $this->assertStringContainsString('0.05', $store->reports[0]); + } + + public function test_a_lock_that_is_acquired_normally_logs_nothing(): void + { + $store = $this->loggingStore(); + + $this->assertSame('ok', $store->synchronized($this->id(), fn () => 'ok')); + $this->assertSame([], $store->reports); + } + + public function test_an_unusable_lock_directory_is_logged_and_the_callback_still_runs(): void + { + // A file where the lock directory should be makes it impossible to create. + mkdir($this->path, 0700, true); + file_put_contents($this->path . '/.locks', 'in the way'); + + $store = $this->loggingStore(); + + $this->assertSame('ran', $store->synchronized($this->id(), fn () => 'ran')); + $this->assertCount(1, $store->reports); + $this->assertStringContainsString('lock directory', $store->reports[0]); + + unlink($this->path . '/.locks'); + } + + public function test_the_lock_is_released_when_the_callback_throws(): void + { + $store = $this->loggingStore(); + $id = $this->id(); + + try { + $store->synchronized($id, function () { + throw new \RuntimeException('boom'); + }); + } catch (\RuntimeException) { + } + + $this->assertSame('free', $store->synchronized($id, fn () => 'free')); + $this->assertSame([], $store->reports); + } + + public function test_destroy_waits_for_the_session_lock(): void + { + $id = $this->id(); + $store = new FileStore($this->path); + $store->write($id, ['user' => 1]); + + // A request is saving the session. A logout must wait for it, or the + // save could land after the destroy and bring the session back. + $stored = null; + + $store->synchronized($id, function () use ($id, &$stored) { + $script = sprintf( + 'require %s; (new Phaseolies\\Session\\Stores\\FileStore(%s, 7200, 10))->destroy(%s);', + var_export(dirname(__DIR__, 2) . '/vendor/autoload.php', true), + var_export($this->path, true), + var_export($id, true) + ); + + $process = proc_open([PHP_BINARY, '-r', $script], [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes); + $this->assertIsResource($process); + + // Give the child time to reach the lock and block on it. + usleep(600000); + + $stored = file_exists($this->path . '/' . substr($id, 0, 2) . '/' . $id); + + $this->process = [$process, $pipes]; + }); + + [$process, $pipes] = $this->process; + stream_get_contents($pipes[1]); + stream_get_contents($pipes[2]); + proc_close($process); + + $this->assertTrue($stored, 'destroy() removed the session while another request held its lock.'); + $this->assertNull($store->read($id), 'destroy() never ran once the lock was released.'); + } + + /** @var array{0: resource, 1: array}|null */ + private ?array $process = null; } diff --git a/tests/Session/SessionTest.php b/tests/Session/SessionTest.php index 01923eae..4d7be46a 100644 --- a/tests/Session/SessionTest.php +++ b/tests/Session/SessionTest.php @@ -468,6 +468,235 @@ public function test_an_invalidated_session_is_not_merged_with_concurrent_change $this->assertSame([], $this->storedFiles()); } + /** + * Two requests that loaded the same stored session, which already holds the given data. + */ + private function twoRequestsOn(array $data): array + { + $first = $this->session(); + $first->put($data); + $cookie = $this->sessionCookie($this->commit($first))->getValue(); + + $make = fn() => $this->session($this->request('/', [self::COOKIE => $cookie])); + $a = $make(); + $b = $make(); + $a->all(); + $b->all(); + + return [$cookie, $a, $b]; + } + + private function stored(string $cookie): array + { + return $this->session($this->request('/', [self::COOKIE => $cookie]))->all(); + } + + public function test_concurrent_appends_to_the_same_list_are_all_kept(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['cart' => ['x']]); + + $a->put('cart', ['x', 'apple']); + $b->put('cart', ['x', 'pear']); + $this->commit($a); + $this->commit($b); + + $cart = $this->stored($cookie)['cart']; + + $this->assertTrue(array_is_list($cart)); + $this->assertSame(['apple', 'pear', 'x'], $this->sorted($cart)); + } + + public function test_concurrent_appends_are_kept_whichever_request_saves_first(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['cart' => ['x']]); + + $a->put('cart', ['x', 'apple']); + $b->put('cart', ['x', 'pear']); + $this->commit($b); + $this->commit($a); + + $this->assertSame(['apple', 'pear', 'x'], $this->sorted($this->stored($cookie)['cart'])); + } + + public function test_a_concurrent_removal_and_append_on_a_list_are_both_applied(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['cart' => ['x', 'y']]); + + $a->put('cart', ['y']); + $b->put('cart', ['x', 'y', 'z']); + $this->commit($a); + $this->commit($b); + + $this->assertSame(['y', 'z'], $this->sorted($this->stored($cookie)['cart'])); + } + + public function test_concurrent_appends_of_arrays_to_a_list_are_all_kept(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['cart' => [['id' => 1, 'qty' => 1]]]); + + $a->put('cart', [['id' => 1, 'qty' => 1], ['id' => 2, 'qty' => 1]]); + $b->put('cart', [['id' => 1, 'qty' => 1], ['id' => 3, 'qty' => 1]]); + $this->commit($a); + $this->commit($b); + + $ids = array_column($this->stored($cookie)['cart'], 'id'); + sort($ids); + + $this->assertSame([1, 2, 3], $ids); + } + + public function test_concurrent_appends_of_objects_to_a_list_are_all_kept(): void + { + // Objects read back from storage are new instances, so they must be + // matched by content and not by identity. + $item = fn (int $id) => (object) ['id' => $id]; + + [$cookie, $a, $b] = $this->twoRequestsOn(['cart' => [$item(1)]]); + + $a->put('cart', [$a->get('cart')[0], $item(2)]); + $b->put('cart', [$b->get('cart')[0], $item(3)]); + $this->commit($a); + $this->commit($b); + + $ids = array_map(fn ($o) => $o->id, $this->stored($cookie)['cart']); + sort($ids); + + $this->assertSame([1, 2, 3], $ids); + } + + public function test_two_requests_appending_the_same_value_each_add_it(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['cart' => ['apple']]); + + $a->put('cart', ['apple', 'apple']); + $b->put('cart', ['apple', 'apple']); + $this->commit($a); + $this->commit($b); + + $this->assertSame(['apple', 'apple', 'apple'], $this->stored($cookie)['cart']); + } + + public function test_two_requests_that_both_create_a_list_keep_both_items(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['user' => 1]); + + $a->put('basket', ['apple']); + $b->put('basket', ['pear']); + $this->commit($a); + $this->commit($b); + + $this->assertSame(['apple', 'pear'], $this->sorted($this->stored($cookie)['basket'])); + } + + public function test_a_list_nested_in_an_array_is_merged_the_same_way(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['basket' => ['items' => ['x'], 'currency' => 'usd']]); + + $a->put('basket', ['items' => ['x', 'apple'], 'currency' => 'usd']); + $b->put('basket', ['items' => ['x', 'pear'], 'currency' => 'eur']); + $this->commit($a); + $this->commit($b); + + $basket = $this->stored($cookie)['basket']; + + $this->assertSame(['apple', 'pear', 'x'], $this->sorted($basket['items'])); + $this->assertSame('eur', $basket['currency']); + } + + public function test_a_list_changed_by_one_request_only_is_written_as_that_request_left_it(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['cart' => ['x'], 'other' => 1]); + + $a->put('cart', ['z', 'x']); + $b->put('other', 2); + $this->commit($b); + $this->commit($a); + + $stored = $this->stored($cookie); + + $this->assertSame(['z', 'x'], $stored['cart']); + $this->assertSame(2, $stored['other']); + } + + public function test_a_request_that_finishes_after_logout_does_not_bring_the_session_back(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['user' => 1]); + + // A is the logout request, B was already running on the same session. + $a->invalidate(); + $this->commit($a); + + $b->put('cart', ['apple']); + $late = $this->commit($b); + + $this->assertNull($this->store()->read($cookie), 'The destroyed session was written back.'); + $this->assertSame([], $this->storedFiles()); + $this->assertNull($this->sessionCookie($late), 'The old session cookie was sent again.'); + $this->assertSame([], $this->stored($cookie)); + } + + public function test_a_request_that_finishes_after_logout_does_not_bring_the_session_back_either_way_round(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['user' => 1]); + + $b->put('cart', ['apple']); + $this->commit($b); + + $a->invalidate(); + $this->commit($a); + + $this->assertNull($this->store()->read($cookie)); + $this->assertSame([], $this->storedFiles()); + } + + public function test_a_request_on_a_session_that_was_destroyed_meanwhile_does_not_recreate_it(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['user' => 1]); + + $a->destroy(); + $this->commit($a); + + $b->put('flash', 'late'); + $this->commit($b); + + $this->assertNull($this->store()->read($cookie)); + $this->assertSame([], $this->storedFiles()); + } + + public function test_a_request_that_finishes_after_the_id_was_rotated_does_not_clobber_the_new_session(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['user' => 1]); + + // A is a login style request that rotates the id, B is still on the old one. + $a->regenerate(); + $a->put('rotated', true); + $rotated = $this->sessionCookie($this->commit($a))->getValue(); + + $b->put('cart', ['apple']); + $late = $this->commit($b); + + $this->assertNull($this->store()->read($cookie), 'The rotated-away id came back.'); + $this->assertNotNull($this->store()->read($rotated)); + $this->assertTrue($this->stored($rotated)['rotated']); + + // B must not send a cookie, which would replace or expire the one A issued. + $this->assertNull($this->sessionCookie($late)); + } + + public function test_an_unchanged_request_still_slides_the_session(): void + { + [$cookie, $a] = $this->twoRequestsOn(['user' => 1]); + + $this->assertSame($cookie, $this->sessionCookie($this->commit($a))->getValue()); + } + + private function sorted(array $values): array + { + sort($values); + + return $values; + } + public function test_set_id_removes_the_session_it_replaces(): void { $session = $this->session(); From e9a49ec5412d4abced288cfa4a3cc0b16d05118a Mon Sep 17 00:00:00 2001 From: techmahedy Date: Tue, 3 Nov 2026 12:26:46 +0600 Subject: [PATCH 5/6] fix: rotate session ids under the lock with a short-lived pointer, so overlapping requests keep one session and a logout stays logged out --- src/Phaseolies/Session/SessionManager.php | 19 ++ src/Phaseolies/Session/Stores/FileStore.php | 27 +- src/Phaseolies/Support/Session.php | 351 +++++++++++++++---- tests/Session/FileStoreTest.php | 52 +++ tests/Session/RotationTest.php | 353 ++++++++++++++++++++ tests/Session/SessionManagerTest.php | 29 ++ tests/Session/SessionTest.php | 45 ++- 7 files changed, 816 insertions(+), 60 deletions(-) create mode 100644 tests/Session/RotationTest.php diff --git a/src/Phaseolies/Session/SessionManager.php b/src/Phaseolies/Session/SessionManager.php index 93b33243..7e266a72 100644 --- a/src/Phaseolies/Session/SessionManager.php +++ b/src/Phaseolies/Session/SessionManager.php @@ -83,10 +83,29 @@ public static function normalize(array $config): array $config['lifetime'] = max(1, (int) $config['lifetime']); $config['cookie'] = self::cookieName((string) $config['cookie']); + $config['lottery'] = self::lottery($config['lottery']); return $config; } + /** + * Turn the lottery option into [chances, out of]. + * + * @param mixed $value + * @return array{0: int, 1: int} + */ + public static function lottery(mixed $value): array + { + if (!is_array($value) || $value === []) { + return [0, 100]; + } + + $value = array_values($value); + $total = (int) ($value[1] ?? 100); + + return [max(0, (int) $value[0]), $total > 0 ? $total : 100]; + } + /** * Make a cookie name safe to send and to read back. * diff --git a/src/Phaseolies/Session/Stores/FileStore.php b/src/Phaseolies/Session/Stores/FileStore.php index 90ca6d7e..3bfc97ad 100644 --- a/src/Phaseolies/Session/Stores/FileStore.php +++ b/src/Phaseolies/Session/Stores/FileStore.php @@ -13,7 +13,18 @@ class FileStore implements LockableSessionStore * Anything else coming from a cookie is rejected before it can reach the * filesystem, which rules out path traversal and fixation with crafted ids. */ - public const ID_PATTERN = '/^[a-f0-9]{40}$/'; + public const ID_PATTERN = '/^[a-f0-9]{40}\z/'; + + /** + * The shards this instance holds the lock of right now. + * + * A callback that runs under a lock may call back into the store, for + * example to destroy the session it is looking at. Taking the same lock a + * second time would only wait for itself, so a nested call reuses it. + * + * @var array + */ + private array $held = []; /** * @param string $path @@ -134,13 +145,19 @@ public function synchronized(string $id, callable $callback): mixed return $callback(); } + $shard = substr($id, 0, 2); + + if (isset($this->held[$shard])) { + return $callback(); + } + if (!is_dir($directory) && !@mkdir($directory, 0700, true) && !is_dir($directory)) { $this->report('Session locking is unavailable: the lock directory could not be created. Saving without a lock.'); return $callback(); } - $handle = @fopen($directory . '/' . substr($id, 0, 2), 'c'); + $handle = @fopen($directory . '/' . $shard, 'c'); if ($handle === false) { $this->report('Session locking is unavailable: the lock file could not be opened. Saving without a lock.'); @@ -165,9 +182,13 @@ public function synchronized(string $id, callable $callback): mixed usleep(5000); } + $this->held[$shard] = true; + try { return $callback(); } finally { + unset($this->held[$shard]); + flock($handle, LOCK_UN); fclose($handle); } @@ -260,7 +281,7 @@ public function gc(int $maxLifetime): int foreach (new \FilesystemIterator($this->path, \FilesystemIterator::SKIP_DOTS) as $entry) { $name = $entry->getFilename(); - if ($entry->isDir() && preg_match('/^[a-f0-9]{2}$/', $name) === 1) { + if ($entry->isDir() && preg_match('/^[a-f0-9]{2}\z/', $name) === 1) { $removed += $this->sweepDirectory($entry->getPathname(), $cutoff); } elseif ($entry->isFile() && str_starts_with($name, 'sess_') && $entry->getMTime() <= $cutoff && @unlink($entry->getPathname())) { // Left behind by the native PHP session handler this store replaced. diff --git a/src/Phaseolies/Support/Session.php b/src/Phaseolies/Support/Session.php index 8d77a220..b6677952 100644 --- a/src/Phaseolies/Support/Session.php +++ b/src/Phaseolies/Support/Session.php @@ -21,6 +21,24 @@ class Session implements SessionInterface, CommittableSession */ protected const INTERNAL_KEYS = ['last_regenerated' => true]; + /** + * Keys of the pointer an automatically rotated id leaves behind. + * + */ + protected const ROTATED_TO = '_rotated_to'; + + protected const ROTATED_UNTIL = '_rotated_until'; + + /** + * Seconds the old id of an automatic rotation keeps leading to the new one. + */ + protected const ROTATION_GRACE = 60; + + /** + * The most pointers followed before giving up, which only matters for a loop. + */ + protected const MAX_HOPS = 3; + /** * The session data. * @@ -92,6 +110,11 @@ class Session implements SessionInterface, CommittableSession */ protected bool $forgetCookie = false; + /** + * Whether the stored session was due for an automatic rotation when loaded. + */ + protected bool $rotationDue = false; + /** * The request the session belongs to. */ @@ -165,7 +188,7 @@ public function isStateless(): bool foreach ($patterns as $pattern) { $regex = str_replace('\\*', '.*', preg_quote(trim((string) $pattern, '/'), '#')); - if (preg_match('#^' . $regex . '$#i', $path) === 1) { + if (preg_match('#^' . $regex . '\z#i', $path) === 1) { return $this->disabled = true; } } @@ -189,17 +212,7 @@ public function commit(?Response $response = null): void $config = $this->config(); if ($this->isBlank()) { - $stale = $this->previousId ?? $this->storedId; - - if ($stale !== null) { - $store->destroy($stale); - } - - if ($stale !== null || $this->forgetCookie || $this->incomingCookie() !== null) { - $this->queueCookie($response, $this->expiredCookie($config)); - } - - $this->storedId = $this->previousId = null; + $this->commitBlank($store, $config, $response); return; } @@ -208,40 +221,49 @@ public function commit(?Response $response = null): void $this->data['last_regenerated'] ??= time(); $serialized = serialize($this->data); - $changed = $this->storedId !== $this->id || $serialized !== $this->snapshot; + $changed = $this->storedId !== $this->id || $serialized !== $this->snapshot || $this->rotationDue; if ($changed || $store->isClientSide()) { if ($store instanceof LockableSessionStore && $this->storedId !== null) { - // Another request may have saved this session since it was - // loaded. Apply only what this request changed on top of it. - $value = $store->synchronized($this->storedId, function () use ($store): ?string { - if (!$this->mergeConcurrentChanges($store)) { - return null; - } - - return $store->write($this->id, $this->data); - }); + // Another request may have saved, rotated or ended this session + // since it was loaded. Apply only what this request changed on + // top of what is stored now. + $value = $this->saveLocked($store); if ($value === null) { - // Logged out, destroyed or rotated by another request while + // Logged out, destroyed or expired by another request while // this one was running. Writing it back would undo that, and // sending its cookie would replace the one that request issued. $this->abandon(); return; } - - $serialized = serialize($this->data); } else { + if ($this->rotationDue) { + // No lock to coordinate with, so rotate on the spot. + $this->previousId ??= $this->storedId; + $this->id = self::newId(); + $this->data['last_regenerated'] = time(); + } + $value = $store->write($this->id, $this->data); } + + $serialized = serialize($this->data); + } elseif (!$this->stillCurrent($store)) { + // Unchanged, but another request rotated or ended the session while + // this one ran. Sending its id again would overwrite the cookie that + // request issued with one that no longer works. + $this->abandon(); + + return; } else { $store->touch($this->id); $value = $this->id; } if ($this->previousId !== null && $this->previousId !== $this->id) { - $store->destroy($this->previousId); + $this->destroyLineage($store, $this->previousId); } $this->storedId = $this->id; @@ -249,11 +271,57 @@ public function commit(?Response $response = null): void $this->snapshot = $serialized; $this->original = $this->data; $this->replace = false; + $this->rotationDue = false; $this->committed = true; $this->queueCookie($response, $this->makeCookie($value, $config)); } + /** + * Commit a session that holds nothing: end whatever is stored for it. + * + * @param SessionStoreInterface $store + * @param array $config + * @param Response|null $response + * @return void + */ + protected function commitBlank(SessionStoreInterface $store, array $config, ?Response $response): void + { + $ids = array_values(array_unique(array_filter([$this->previousId, $this->storedId]))); + + foreach ($ids as $id) { + $this->destroyLineage($store, $id); + } + + // Only a session this request actually ended has its cookie expired. A + // cookie that merely names an unknown session is left alone: the browser + // may already hold a newer one from a request that finished first, and + // expiring it would sign the user out. + if ($ids !== [] || $this->forgetCookie) { + $this->queueCookie($response, $this->expiredCookie($config)); + } + + $this->storedId = $this->previousId = null; + $this->rotationDue = false; + } + + /** + * Whether the stored session is still the one this request loaded. + * + * @param SessionStoreInterface $store + * @return bool + */ + protected function stillCurrent(SessionStoreInterface $store): bool + { + if (!$store instanceof LockableSessionStore || $this->storedId === null) { + return true; + } + + $record = $store->read($this->storedId); + + return $record !== null && !array_key_exists(self::ROTATED_TO, $record['data']); + } + /** * Run the housekeeping that must not delay the response. * @@ -277,34 +345,215 @@ protected function abandon(): void { $this->storedId = $this->previousId = $this->snapshot = null; $this->original = []; - $this->replace = $this->committed = false; + $this->replace = $this->committed = $this->rotationDue = false; } /** - * Apply this request's changes on top of the session as currently stored. + * Save the session while it is locked, following it if it was rotated. * - * @param SessionStoreInterface $store - * @return bool + * @param LockableSessionStore $store + * @return string|null + */ + protected function saveLocked(LockableSessionStore $store): ?string + { + for ($hop = 0; $hop <= self::MAX_HOPS; $hop++) { + // Only one lock is held at a time: when the session turns out to + // have moved, the lock is released and the next one is taken. + $result = $store->synchronized($this->storedId, fn () => $this->saveUnderLock($store)); + + if (!is_array($result)) { + return null; + } + + [$value, $moved] = $result; + + if (!$moved) { + return $value; + } + } + + return null; + } + + /** + * Save the session. Must run while the session is locked. + * + * @param LockableSessionStore $store + * @return array{0: string|null, 1: bool} */ - protected function mergeConcurrentChanges(SessionStoreInterface $store): bool + protected function saveUnderLock(LockableSessionStore $store): array { $record = $store->read($this->storedId); + if ($record !== null && array_key_exists(self::ROTATED_TO, $record['data'])) { + $target = $this->pointerTarget($record['data']); + + if ($target !== null) { + // Another request rotated the id. Carry on with the new one, so + // this request's changes land in the one live session. + $this->follow($target); + + return [null, true]; + } + + // A pointer past its time is a session that is gone. + $record = null; + } + if ($record === null) { - // The record this request loaded no longer exists: another request - // logged out, destroyed or expired it. Saving under the same id - // would resurrect it, while a new id (regenerate, setId) is a new - // session and is simply created. - return $this->id !== $this->storedId; + // The session this request loaded no longer exists: another request + // logged out, destroyed or expired it. Writing it back would undo that. + // Only data this request built from scratch (flush, invalidate) may + // create a session again, because none of it comes from the old one. + return [$this->replace ? $this->persist($store, false) : null, false]; } - if ($this->replace || serialize($record['data']) === $this->snapshot) { - return true; + if (!$this->replace && serialize($record['data']) !== $this->snapshot) { + $this->data = $this->mergeChanges($record['data'], $this->original, $this->data); } - $this->data = $this->mergeChanges($record['data'], $this->original, $this->data); + return [$this->persist($store, true), false]; + } - return true; + /** + * Write the session, rotating its id first when that is due. + * + * @param LockableSessionStore $store + * @param bool $live + * @return string + */ + protected function persist(LockableSessionStore $store, bool $live): string + { + if (!$live || !$this->rotationDue || $this->replace || $this->id !== $this->storedId || !$this->isDue($this->data)) { + return $store->write($this->id, $this->data); + } + + // Rotate under the lock: write the session under its new id, then turn + // the old one into a pointer for a short time. Requests already running + // on the old id follow it, so none of them splits the session or loses + // its changes, and none is signed out. + $old = $this->storedId; + $new = self::newId(); + + $this->data['last_regenerated'] = time(); + + $store->write($new, $this->data); + $store->write($old, [self::ROTATED_TO => $new, self::ROTATED_UNTIL => time() + self::ROTATION_GRACE]); + + return $this->id = $new; + } + + /** + * Move this request on to the session an automatic rotation created. + * + * @param string $to + * @return void + */ + protected function follow(string $to): void + { + $from = $this->storedId; + + $this->id = $this->id === $from ? $to : $this->id; + $this->previousId = $this->previousId === $from ? $to : $this->previousId; + $this->storedId = $to; + } + + /** + * The id an old id leads to, while it is still allowed to. + * + * @param array $data + * @param bool $honourExpiry + * @return string|null + */ + protected function pointerTarget(array $data, bool $honourExpiry = true): ?string + { + $target = $data[self::ROTATED_TO] ?? null; + + if (!is_string($target) || !FileStore::isValidId($target)) { + return null; + } + + if ($honourExpiry && (int) ($data[self::ROTATED_UNTIL] ?? 0) < time()) { + return null; + } + + return $target; + } + + /** + * Read a session by id, following the pointer of an automatically rotated one. + * + * @param SessionStoreInterface $store + * @param string $id + * @return array{id: string, data: array}|null + */ + protected function readFollowing(SessionStoreInterface $store, string $id): ?array + { + for ($hop = 0; $hop <= self::MAX_HOPS; $hop++) { + $record = $store->read($id); + + if ($record === null || !array_key_exists(self::ROTATED_TO, $record['data'])) { + return $record; + } + + $target = $this->pointerTarget($record['data']); + + if ($target === null) { + $store->destroy($id); + + return null; + } + + $id = $target; + } + + return null; + } + + /** + * End a session and every session an automatic rotation made from it. + * + * @param SessionStoreInterface $store + * @param string $id + * @return void + */ + protected function destroyLineage(SessionStoreInterface $store, string $id): void + { + for ($hop = 0; $hop <= self::MAX_HOPS; $hop++) { + if (!$store instanceof LockableSessionStore) { + $store->destroy($id); + + return; + } + + // Read and destroy under one lock, so a rotation cannot slip in + // between and leave a live successor behind. + $next = $store->synchronized($id, function () use ($store, $id): ?string { + $record = $store->read($id); + + $store->destroy($id); + + return $record === null ? null : $this->pointerTarget($record['data'], false); + }); + + if (!is_string($next)) { + return; + } + + $id = $next; + } + } + + /** + * Whether the id is due for an automatic rotation. + * + * @param array $data + * @return bool + */ + protected function isDue(array $data): bool + { + return !isset($data['last_regenerated']) + || (time() - (int) $data['last_regenerated']) > ($this->config()['lifetime'] * 60); } /** @@ -459,33 +708,22 @@ protected function load(): void if (!$this->isStateless()) { $value = $this->incomingCookie(); - $record = $value === null ? null : $this->store()->read($value); + $record = $value === null ? null : $this->readFollowing($this->store(), $value); if ($record !== null) { $this->id = $this->storedId = $record['id']; $this->data = $this->original = $record['data']; $this->snapshot = serialize($this->data); - if ($this->shouldRotate()) { - $this->regenerate(true); - } + // Rotated when saved, under the lock, not here: several requests + // loading the same session must not each pick their own new id. + $this->rotationDue = $this->isDue($this->data); } } $this->peek = array_replace($this->data, $this->peek); } - /** - * Whether the session id is due for rotation. - * - * @return bool - */ - protected function shouldRotate(): bool - { - return !isset($this->data['last_regenerated']) - || (time() - (int) $this->data['last_regenerated']) > ($this->config()['lifetime'] * 60); - } - /** * Whether the session holds nothing worth storing. * @@ -766,6 +1004,7 @@ public function regenerate(bool $deleteOldSession = true): void $this->id = self::newId(); $this->data['last_regenerated'] = time(); + $this->rotationDue = false; } /** @@ -813,7 +1052,7 @@ public function setId(string $id): void $this->previousId ??= $this->storedId; } - if (!$store->isClientSide() && !$this->isStateless() && ($record = $store->read($id)) !== null) { + if (!$store->isClientSide() && !$this->isStateless() && ($record = $this->readFollowing($store, $id)) !== null) { $this->data = $this->original = $record['data']; $this->storedId = $record['id']; $this->snapshot = serialize($this->data); diff --git a/tests/Session/FileStoreTest.php b/tests/Session/FileStoreTest.php index 6efc26c4..0d41b61e 100644 --- a/tests/Session/FileStoreTest.php +++ b/tests/Session/FileStoreTest.php @@ -397,4 +397,56 @@ public function test_destroy_waits_for_the_session_lock(): void /** @var array{0: resource, 1: array}|null */ private ?array $process = null; + + public function test_an_id_that_ends_in_a_newline_is_not_a_valid_id(): void + { + // "$" lets a trailing newline through, which "\z" does not. + $store = new FileStore($this->path); + $id = $this->id(); + $store->write($id, ['a' => 1]); + + $this->assertTrue(FileStore::isValidId($id)); + $this->assertFalse(FileStore::isValidId($id . "\n")); + $this->assertNull($store->read($id . "\n")); + + $store->destroy($id . "\n"); + $this->assertNotNull($store->read($id), 'A look-alike id destroyed the real session.'); + + $this->expectException(SessionException::class); + $store->write($id . "\n", ['a' => 1]); + } + + public function test_a_callback_under_a_lock_can_use_the_store_without_waiting_for_itself(): void + { + $store = $this->loggingStore(5.0); + $id = $this->id(); + $store->write($id, ['a' => 1]); + + $started = microtime(true); + + $store->synchronized($id, function () use ($store, $id) { + // Nested, same session: must reuse the lock instead of waiting out the timeout. + $store->synchronized($id, fn () => null); + $store->destroy($id); + }); + + $this->assertLessThan(1.0, microtime(true) - $started, 'The nested call waited for the lock it already holds.'); + $this->assertNull($store->read($id)); + $this->assertSame([], $store->reports); + } + + public function test_the_lock_is_free_again_after_a_nested_call(): void + { + $store = $this->loggingStore(0.05); + $id = $this->id(); + + $store->synchronized($id, fn () => $store->synchronized($id, fn () => null)); + + $held = $this->holdLockOf($id); + flock($held, LOCK_UN); + fclose($held); + + $this->assertSame('ok', $store->synchronized($id, fn () => 'ok')); + $this->assertSame([], $store->reports); + } } diff --git a/tests/Session/RotationTest.php b/tests/Session/RotationTest.php new file mode 100644 index 00000000..dd59470e --- /dev/null +++ b/tests/Session/RotationTest.php @@ -0,0 +1,353 @@ +path = sys_get_temp_dir() . '/doppar_rotation_' . bin2hex(random_bytes(6)); + $this->config = [ + 'cookie' => self::COOKIE, + 'lifetime' => 120, + 'stateless' => [], + 'lottery' => [0, 100], + 'secure' => false, + ]; + } + + protected function tearDown(): void + { + if (!is_dir($this->path)) { + return; + } + + $entries = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($this->path, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST + ); + + foreach ($entries as $entry) { + $entry->isDir() ? @rmdir($entry->getPathname()) : @unlink($entry->getPathname()); + } + + @rmdir($this->path); + } + + private function store(): FileStore + { + return new FileStore($this->path, 7200); + } + + private function request(array $cookies = []): Request + { + $request = new class extends Request { + public function __construct() + { + } + + public function getPath(): string + { + return '/'; + } + }; + + $request->cookies = new InputBag($cookies); + $request->server = new ServerBag([]); + + return $request; + } + + private function session(string $cookie, $store = null): Session + { + $session = new Session($store ?? $this->store(), $this->config); + $session->attach($this->request([self::COOKIE => $cookie])); + + return $session; + } + + private function commit(Session $session): Response + { + $response = new Response(); + $session->commit($response); + + return $response; + } + + private function cookie(Response $response): ?Cookie + { + foreach ($response->headers->getCookies() as $cookie) { + if ($cookie->getName() === self::COOKIE) { + return $cookie; + } + } + + return null; + } + + private function dueSession(array $data = ['user' => 7], $store = null): string + { + $id = bin2hex(random_bytes(20)); + ($store ?? $this->store())->write($id, $data + ['last_regenerated' => time() - 99999]); + + return $id; + } + + /** + * Every stored record that holds session data, which excludes the + * short lived pointers an old id leaves behind. + * + * @return array + */ + private function liveRecords(): array + { + $records = []; + + foreach (glob($this->path . '/*/*') ?: [] as $file) { + $data = @unserialize((string) file_get_contents($file)); + + if (is_array($data) && !isset($data['_rotated_to'])) { + $records[basename($file)] = $data; + } + } + + return $records; + } + + public function test_a_logout_that_overlaps_an_automatic_rotation_is_not_undone(): void + { + $id = $this->dueSession(); + $logout = $this->session($id); + $rotating = $this->session($id); + $logout->all(); + $rotating->all(); + + $logout->invalidate(); + $this->commit($logout); + + $late = $this->commit($rotating); + + foreach ($this->liveRecords() as $record) { + $this->assertArrayNotHasKey('user', $record, 'The logged-out user is logged in again.'); + } + + $this->assertNull($this->cookie($late), 'The late request sent a session cookie.'); + } + + public function test_a_logout_that_arrives_after_an_automatic_rotation_ends_the_session(): void + { + $id = $this->dueSession(); + $logout = $this->session($id); + $rotating = $this->session($id); + $logout->all(); + $rotating->all(); + + $this->commit($rotating); + + $logout->invalidate(); + $this->commit($logout); + + foreach ($this->liveRecords() as $record) { + $this->assertArrayNotHasKey('user', $record, 'The rotation survived the logout.'); + } + } + + public function test_destroying_a_session_that_was_rotated_meanwhile_ends_the_whole_lineage(): void + { + $id = $this->dueSession(); + $destroy = $this->session($id); + $rotating = $this->session($id); + $destroy->all(); + $rotating->all(); + + $rotated = $this->cookie($this->commit($rotating))->getValue(); + + $destroy->destroy(); + $this->commit($destroy); + + $this->assertNull($this->store()->read($rotated), 'The rotated session outlived the destroy.'); + $this->assertSame([], $this->liveRecords()); + } + + public function test_parallel_requests_on_a_session_due_for_rotation_keep_one_session(): void + { + $id = $this->dueSession(['user' => 7, 'cart' => ['x']]); + + $requests = []; + + foreach (['a', 'b', 'c'] as $name) { + $requests[$name] = $this->session($id); + $requests[$name]->all(); + } + + $requests['a']->put('flash_a', 'a'); + $requests['b']->put('flash_b', 'b'); + $requests['c']->put('cart', ['x', 'apple']); + + $cookies = []; + + foreach ($requests as $session) { + $cookies[] = $this->cookie($this->commit($session))->getValue(); + } + + $this->assertCount(1, array_unique($cookies), 'The requests split the session over several ids.'); + $this->assertNotSame($id, $cookies[0], 'The session id was not rotated.'); + + $live = $this->liveRecords(); + + $this->assertCount(1, $live, 'More than one copy of the session is stored.'); + + $data = $live[$cookies[0]]; + + $this->assertSame(7, $data['user']); + $this->assertSame('a', $data['flash_a']); + $this->assertSame('b', $data['flash_b']); + $this->assertSame(['x', 'apple'], $data['cart']); + } + + public function test_a_request_that_still_sends_the_old_cookie_follows_the_rotation(): void + { + $id = $this->dueSession(); + $rotating = $this->session($id); + $rotating->all(); + $new = $this->cookie($this->commit($rotating))->getValue(); + + $late = $this->session($id); + + $this->assertSame(7, $late->get('user'), 'The old id did not lead to the session.'); + + $response = $this->commit($late); + + $this->assertSame($new, $this->cookie($response)->getValue()); + $this->assertGreaterThan(time(), $this->cookie($response)->getExpiresTime(), 'The user was logged out.'); + } + + public function test_the_old_id_stops_working_when_the_grace_period_is_over(): void + { + $id = $this->dueSession(); + $rotating = $this->session($id); + $rotating->all(); + $this->commit($rotating); + + // Let the pointer left on the old id expire. + $this->store()->write($id, ['_rotated_to' => bin2hex(random_bytes(20)), '_rotated_until' => time() - 1]); + + $this->assertNull($this->session($id)->get('user')); + } + + public function test_a_pointer_to_a_session_that_is_gone_leads_nowhere(): void + { + $id = $this->dueSession(); + $rotating = $this->session($id); + $rotating->all(); + $new = $this->cookie($this->commit($rotating))->getValue(); + + $this->store()->destroy($new); + + $this->assertNull($this->session($id)->get('user')); + } + + public function test_an_explicit_regenerate_leaves_no_pointer_to_the_new_session(): void + { + // The old id of a login must never lead to the session that logged in: + // that id may have been planted by an attacker (session fixation). + $id = $this->dueSession(['user' => 7]); + $login = $this->session($id); + $login->regenerate(); + $new = $this->cookie($this->commit($login))->getValue(); + + $this->assertNotSame($id, $new); + $this->assertNull($this->store()->read($id), 'The old id still exists.'); + $this->assertNull($this->session($id)->get('user')); + } + + public function test_a_request_that_loaded_a_due_session_and_saves_late_sends_the_new_id(): void + { + $id = $this->dueSession(); + $reader = $this->session($id); + $rotating = $this->session($id); + $reader->all(); + $rotating->all(); + + $new = $this->cookie($this->commit($rotating))->getValue(); + + // It follows the rotation instead of sending the id that was rotated away. + $this->assertSame($new, $this->cookie($this->commit($reader))->getValue()); + } + + public function test_a_read_only_request_does_not_send_an_id_that_was_rotated_after_it_loaded(): void + { + // The session is not due yet when the reader loads it ... + $id = $this->dueSession(['user' => 7, 'last_regenerated' => time() - 100]); + $reader = $this->session($id); + $reader->all(); + + // ... and becomes due, and is rotated by another request, before it finishes. + $this->store()->write($id, ['user' => 7, 'last_regenerated' => time() - 99999]); + $rotating = $this->session($id); + $rotating->all(); + $new = $this->cookie($this->commit($rotating))->getValue(); + + // Sending the old id would overwrite the cookie of the rotated session. + $this->assertNotSame($id, $new); + $this->assertNull($this->cookie($this->commit($reader))); + } + + public function test_a_read_only_request_still_rotates_a_session_that_is_due(): void + { + $id = $this->dueSession(); + $session = $this->session($id); + $session->get('user'); + + $new = $this->cookie($this->commit($session))->getValue(); + + $this->assertNotSame($id, $new); + $this->assertSame(7, $this->store()->read($new)['data']['user']); + } + + public function test_a_rotated_session_is_not_rotated_again_on_its_next_request(): void + { + $id = $this->dueSession(); + $first = $this->session($id); + $first->all(); + $new = $this->cookie($this->commit($first))->getValue(); + + $second = $this->session($new); + $second->put('x', 1); + + $this->assertSame($new, $this->cookie($this->commit($second))->getValue()); + } + + public function test_a_store_that_cannot_lock_rotates_immediately_and_forgets_the_old_id(): void + { + $store = new ArrayStore(); + $id = $this->dueSession(['user' => 7], $store); + + $session = $this->session($id, $store); + $session->all(); + $new = $this->cookie($this->commit($session))->getValue(); + + $this->assertNotSame($id, $new); + $this->assertNull($store->read($id)); + $this->assertSame(7, $store->read($new)['data']['user']); + } +} diff --git a/tests/Session/SessionManagerTest.php b/tests/Session/SessionManagerTest.php index 4dab5156..d335b449 100644 --- a/tests/Session/SessionManagerTest.php +++ b/tests/Session/SessionManagerTest.php @@ -32,4 +32,33 @@ public static function names(): array 'only junk falls back' => [' ', 'doppar_session'], ]; } + + #[DataProvider('lotteries')] + public function test_the_lottery_option_is_normalized(mixed $input, array $expected): void + { + $this->assertSame($expected, SessionManager::lottery($input)); + $this->assertSame($expected, SessionManager::normalize(['lottery' => $input])['lottery']); + } + + public static function lotteries(): array + { + return [ + 'default' => [[2, 100], [2, 100]], + 'null never sweeps' => [null, [0, 100]], + 'false never sweeps' => [false, [0, 100]], + 'empty array never sweeps' => [[], [0, 100]], + 'zero chances never sweeps' => [[0, 100], [0, 100]], + 'not an array never sweeps' => ['2/100', [0, 100]], + 'a single number keeps the default total' => [[5], [5, 100]], + 'a bad total falls back' => [[2, 0], [2, 100]], + 'negative chances never sweep' => [[-3, 100], [0, 100]], + 'string numbers are read' => [['3', '50'], [3, 50]], + 'every request' => [[1, 1], [1, 1]], + ]; + } + + public function test_a_config_without_a_lottery_gets_the_default(): void + { + $this->assertSame([2, 100], SessionManager::normalize([])['lottery']); + } } diff --git a/tests/Session/SessionTest.php b/tests/Session/SessionTest.php index 4d7be46a..3fdbb324 100644 --- a/tests/Session/SessionTest.php +++ b/tests/Session/SessionTest.php @@ -317,8 +317,11 @@ public function test_an_old_session_id_is_rotated_but_keeps_its_data(): void $new = $this->sessionCookie($this->commit($session))->getValue(); $this->assertNotSame($id, $new); - $this->assertNull($this->store()->read($id)); $this->assertSame(7, $this->store()->read($new)['data']['user']); + + // The old id only leads to the new one for a short while. + $this->assertSame($new, $this->store()->read($id)['data']['_rotated_to']); + $this->assertSame(7, $this->session($this->request('/', [self::COOKIE => $id]))->get('user')); } public function test_an_unchanged_session_is_not_rewritten(): void @@ -713,6 +716,46 @@ public function test_set_id_removes_the_session_it_replaces(): void $this->assertNotNull($this->store()->read($new)); } + #[\PHPUnit\Framework\Attributes\DataProvider('disabledLotteries')] + public function test_a_disabled_lottery_never_sweeps(mixed $lottery): void + { + $this->config['lottery'] = $lottery; + $store = new class($this->path, 7200) extends FileStore { + public int $sweeps = 0; + + public function sweep(int $maxLifetime): int + { + $this->sweeps++; + + return 0; + } + }; + + for ($i = 0; $i < 60; $i++) { + $session = $this->session(null, $store); + $session->put('a', $i); + $this->commit($session); + $session->terminate(); + } + + $this->assertSame(0, $store->sweeps, 'The sweep ran although the lottery is switched off.'); + } + + public static function disabledLotteries(): array + { + return ['null' => [null], 'empty array' => [[]], 'false' => [false], 'zero chances' => [[0, 100]]]; + } + + public function test_set_id_ignores_an_id_that_ends_in_a_newline(): void + { + $session = $this->session(); + $before = $session->getId(); + + $session->setId(bin2hex(random_bytes(20)) . "\n"); + + $this->assertSame($before, $session->getId()); + } + public function test_the_sweep_runs_after_the_response_not_while_saving(): void { $this->config['lottery'] = [1, 1]; From 99373d529bd443b825dd95958b7887c061456f06 Mon Sep 17 00:00:00 2001 From: techmahedy Date: Wed, 4 Nov 2026 12:06:03 +0600 Subject: [PATCH 6/6] fix: apply a logout even when the request then fails, merge an emptied session instead of deleting it, save the session after the error page renders --- src/Phaseolies/Application.php | 100 +++++++- src/Phaseolies/Http/Support/RequestParser.php | 7 +- .../Session/Contracts/CommittableSession.php | 8 + src/Phaseolies/Support/Session.php | 136 +++++++--- src/Phaseolies/Utilities/PathMatcher.php | 20 ++ tests/Session/DispatchSessionTest.php | 234 ++++++++++++++++++ tests/Session/PathMatcherTest.php | 99 ++++++++ tests/Session/SessionTest.php | 91 +++++++ 8 files changed, 643 insertions(+), 52 deletions(-) create mode 100644 src/Phaseolies/Utilities/PathMatcher.php create mode 100644 tests/Session/PathMatcherTest.php diff --git a/src/Phaseolies/Application.php b/src/Phaseolies/Application.php index f96d0dd5..14aa21d7 100644 --- a/src/Phaseolies/Application.php +++ b/src/Phaseolies/Application.php @@ -1561,12 +1561,7 @@ public function dispatch($request): DispatchResult return new DispatchResult($this, $request, $response, null, $deferScope, $context); } catch (HttpException $e) { try { - try { - $this->commitSession($request); - } catch (\Throwable) { - } - - Response::dispatchHttpException($e); + $this->renderHttpException($request, $e); } catch (\Throwable $renderFailure) { $this->endDeferScope($deferScope, $renderFailure); $this->endContext($context, $renderFailure); @@ -1584,11 +1579,9 @@ public function dispatch($request): DispatchResult // old input). The global error handler sends it, so the session // must be saved onto that response here, before it is rethrown. if ($e instanceof HttpResponseException && $e->hasResponse()) { - try { - $this->commitSession($request, $e->getResponse()); - } catch (\Throwable) { - // Never replace the exception that is already ending the request. - } + $this->commitSessionSafely($request, $e->getResponse()); + } else { + $this->endSessionSafely($request); } $this->endDeferScope($deferScope, $e); @@ -1614,6 +1607,88 @@ protected function commitSession(Request $request, ?Response $response = null): } } + /** + * Render the error page of an HTTP exception, then save the session. + * + * @param Request $request + * @param HttpException $exception + * @return void + */ + protected function renderHttpException(Request $request, HttpException $exception): void + { + $level = ob_get_level(); + + ob_start(); + + try { + Response::dispatchHttpException($exception); + } catch (\Throwable $failure) { + $this->commitSessionSafely($request); + + while (ob_get_level() > $level) { + ob_end_clean(); + } + + throw $failure; + } + + $this->commitSessionSafely($request); + + while (ob_get_level() > $level) { + ob_end_flush(); + } + } + + /** + * Persist the session without ever replacing the exception that is already ending the request + * + * @param Request $request + * @param Response|null $response + * @return void + */ + protected function commitSessionSafely(Request $request, ?Response $response = null): void + { + try { + $this->commitSession($request, $response); + } catch (\Throwable $failure) { + $this->reportSessionFailure($failure); + } + } + + /** + * Apply a logout of a request that is failing, ignoring everything else. + * + * @param Request $request + * @return void + */ + protected function endSessionSafely(Request $request): void + { + try { + $session = $request->session(); + + if ($session instanceof CommittableSession) { + $session->commitEnd(); + } + } catch (\Throwable $failure) { + $this->reportSessionFailure($failure); + } + } + + /** + * Log a session operation that failed after the request could no longer be told about it + * + * @param \Throwable $failure + * @return void + */ + protected function reportSessionFailure(\Throwable $failure): void + { + try { + error('[Doppar] A session operation failed and its changes were lost: ' . $failure->getMessage(), ['exception' => $failure]); + } catch (\Throwable) { + // Logging must never turn a lost save into a second failure. + } + } + /** * Run the session housekeeping once the response has been sent * @@ -1627,7 +1702,8 @@ protected function terminateSession(Request $request): void if ($session instanceof CommittableSession) { try { $session->terminate(); - } catch (\Throwable) { + } catch (\Throwable $failure) { + $this->reportSessionFailure($failure); } } } diff --git a/src/Phaseolies/Http/Support/RequestParser.php b/src/Phaseolies/Http/Support/RequestParser.php index eb9dc88b..5e887013 100644 --- a/src/Phaseolies/Http/Support/RequestParser.php +++ b/src/Phaseolies/Http/Support/RequestParser.php @@ -3,6 +3,7 @@ namespace Phaseolies\Http\Support; use Phaseolies\Utilities\IpUtils; +use Phaseolies\Utilities\PathMatcher; trait RequestParser { @@ -305,11 +306,7 @@ public function contentLength(): ?int */ public static function is(string $pattern): bool { - $path = request()->uri(); - - $pattern = str_replace('\*', '.*', preg_quote($pattern, '#')); - - return (bool) preg_match('#^' . $pattern . '$#i', $path); + return PathMatcher::matches($pattern, request()->uri()); } /** diff --git a/src/Phaseolies/Session/Contracts/CommittableSession.php b/src/Phaseolies/Session/Contracts/CommittableSession.php index 8ab27090..b2693252 100644 --- a/src/Phaseolies/Session/Contracts/CommittableSession.php +++ b/src/Phaseolies/Session/Contracts/CommittableSession.php @@ -23,6 +23,14 @@ public function attach(Request $request): void; */ public function commit(?Response $response = null): void; + /** + * Apply only what ends the session, for a request that is failing + * + * @param Response|null $response + * @return void + */ + public function commitEnd(?Response $response = null): void; + /** * Turn persistence off for the rest of the request * diff --git a/src/Phaseolies/Support/Session.php b/src/Phaseolies/Support/Session.php index b6677952..e91dec27 100644 --- a/src/Phaseolies/Support/Session.php +++ b/src/Phaseolies/Support/Session.php @@ -11,6 +11,7 @@ use Phaseolies\Session\Contracts\SessionStoreInterface; use Phaseolies\Session\SessionManager; use Phaseolies\Session\Stores\FileStore; +use Phaseolies\Utilities\PathMatcher; class Session implements SessionInterface, CommittableSession { @@ -39,6 +40,17 @@ class Session implements SessionInterface, CommittableSession */ protected const MAX_HOPS = 3; + /** + * How a locked save ended. + */ + protected const SAVED = 'saved'; + + protected const MOVED = 'moved'; + + protected const GONE = 'gone'; + + protected const EMPTIED = 'emptied'; + /** * The session data. * @@ -81,9 +93,16 @@ class Session implements SessionInterface, CommittableSession protected ?string $storedId = null; /** - * An id whose stored data must be removed when the session is committed. + * Ids whose stored data must be removed when the session is committed. + * + * @var string[] + */ + protected array $previousIds = []; + + /** + * Whether this request ended the session (destroy, flush, invalidate). */ - protected ?string $previousId = null; + protected bool $ending = false; /** * The serialized data as it was loaded, to detect changes. @@ -183,12 +202,10 @@ public function isStateless(): bool return false; } - $path = trim((string) parse_url($this->request->getPath(), PHP_URL_PATH), '/'); + $path = (string) parse_url($this->request->getPath(), PHP_URL_PATH); foreach ($patterns as $pattern) { - $regex = str_replace('\\*', '.*', preg_quote(trim((string) $pattern, '/'), '#')); - - if (preg_match('#^' . $regex . '\z#i', $path) === 1) { + if (PathMatcher::matches((string) $pattern, $path)) { return $this->disabled = true; } } @@ -211,7 +228,9 @@ public function commit(?Response $response = null): void $store = $this->store(); $config = $this->config(); - if ($this->isBlank()) { + $blank = $this->isBlank(); + + if ($blank && ($this->ending || $this->storedId === null || !$store instanceof LockableSessionStore)) { $this->commitBlank($store, $config, $response); return; @@ -221,14 +240,20 @@ public function commit(?Response $response = null): void $this->data['last_regenerated'] ??= time(); $serialized = serialize($this->data); - $changed = $this->storedId !== $this->id || $serialized !== $this->snapshot || $this->rotationDue; + $changed = $blank || $this->storedId !== $this->id || $serialized !== $this->snapshot || $this->rotationDue; if ($changed || $store->isClientSide()) { if ($store instanceof LockableSessionStore && $this->storedId !== null) { // Another request may have saved, rotated or ended this session // since it was loaded. Apply only what this request changed on // top of what is stored now. - $value = $this->saveLocked($store); + [$value, $state] = $this->saveLocked($store); + + if ($state === self::EMPTIED) { + $this->commitBlank($store, $config, $response); + + return; + } if ($value === null) { // Logged out, destroyed or expired by another request while @@ -241,7 +266,7 @@ public function commit(?Response $response = null): void } else { if ($this->rotationDue) { // No lock to coordinate with, so rotate on the spot. - $this->previousId ??= $this->storedId; + $this->retire($this->storedId); $this->id = self::newId(); $this->data['last_regenerated'] = time(); } @@ -262,12 +287,15 @@ public function commit(?Response $response = null): void $value = $this->id; } - if ($this->previousId !== null && $this->previousId !== $this->id) { - $this->destroyLineage($store, $this->previousId); + foreach ($this->previousIds as $old) { + if ($old !== $this->id) { + $this->destroyLineage($store, $old); + } } $this->storedId = $this->id; - $this->previousId = null; + $this->previousIds = []; + $this->ending = false; $this->snapshot = $serialized; $this->original = $this->data; $this->replace = false; @@ -277,6 +305,21 @@ public function commit(?Response $response = null): void $this->queueCookie($response, $this->makeCookie($value, $config)); } + /** + * Apply only what ends the session, for a request that is failing. + * + * @param Response|null $response + * @return void + */ + public function commitEnd(?Response $response = null): void + { + if (!$this->loaded || !$this->ending || $this->isStateless()) { + return; + } + + $this->commitBlank($this->store(), $this->config(), $response); + } + /** * Commit a session that holds nothing: end whatever is stored for it. * @@ -287,7 +330,7 @@ public function commit(?Response $response = null): void */ protected function commitBlank(SessionStoreInterface $store, array $config, ?Response $response): void { - $ids = array_values(array_unique(array_filter([$this->previousId, $this->storedId]))); + $ids = array_values(array_unique(array_filter([...$this->previousIds, $this->storedId]))); foreach ($ids as $id) { $this->destroyLineage($store, $id); @@ -301,8 +344,9 @@ protected function commitBlank(SessionStoreInterface $store, array $config, ?Res $this->queueCookie($response, $this->expiredCookie($config)); } - $this->storedId = $this->previousId = null; - $this->rotationDue = false; + $this->storedId = null; + $this->previousIds = []; + $this->ending = $this->rotationDue = false; } /** @@ -336,6 +380,19 @@ public function terminate(): void } } + /** + * Mark an id whose stored data must be removed once the session is saved. + * + * @param string|null $id + * @return void + */ + protected function retire(?string $id): void + { + if ($id !== null && !in_array($id, $this->previousIds, true)) { + $this->previousIds[] = $id; + } + } + /** * Forget that this session is stored, without touching the store or the cookie. * @@ -343,18 +400,19 @@ public function terminate(): void */ protected function abandon(): void { - $this->storedId = $this->previousId = $this->snapshot = null; + $this->storedId = $this->snapshot = null; + $this->previousIds = []; $this->original = []; - $this->replace = $this->committed = $this->rotationDue = false; + $this->replace = $this->committed = $this->rotationDue = $this->ending = false; } /** * Save the session while it is locked, following it if it was rotated. * * @param LockableSessionStore $store - * @return string|null + * @return array{0: string|null, 1: string} */ - protected function saveLocked(LockableSessionStore $store): ?string + protected function saveLocked(LockableSessionStore $store): array { for ($hop = 0; $hop <= self::MAX_HOPS; $hop++) { // Only one lock is held at a time: when the session turns out to @@ -362,24 +420,22 @@ protected function saveLocked(LockableSessionStore $store): ?string $result = $store->synchronized($this->storedId, fn () => $this->saveUnderLock($store)); if (!is_array($result)) { - return null; + return [null, self::GONE]; } - [$value, $moved] = $result; - - if (!$moved) { - return $value; + if ($result[1] !== self::MOVED) { + return $result; } } - return null; + return [null, self::GONE]; } /** * Save the session. Must run while the session is locked. * * @param LockableSessionStore $store - * @return array{0: string|null, 1: bool} + * @return array{0: string|null, 1: string} */ protected function saveUnderLock(LockableSessionStore $store): array { @@ -393,7 +449,7 @@ protected function saveUnderLock(LockableSessionStore $store): array // this request's changes land in the one live session. $this->follow($target); - return [null, true]; + return [null, self::MOVED]; } // A pointer past its time is a session that is gone. @@ -405,14 +461,20 @@ protected function saveUnderLock(LockableSessionStore $store): array // logged out, destroyed or expired it. Writing it back would undo that. // Only data this request built from scratch (flush, invalidate) may // create a session again, because none of it comes from the old one. - return [$this->replace ? $this->persist($store, false) : null, false]; + return $this->replace ? [$this->persist($store, false), self::SAVED] : [null, self::GONE]; } if (!$this->replace && serialize($record['data']) !== $this->snapshot) { $this->data = $this->mergeChanges($record['data'], $this->original, $this->data); } - return [$this->persist($store, true), false]; + if ($this->isBlank()) { + $store->destroy($this->storedId); + + return [null, self::EMPTIED]; + } + + return [$this->persist($store, true), self::SAVED]; } /** @@ -454,7 +516,7 @@ protected function follow(string $to): void $from = $this->storedId; $this->id = $this->id === $from ? $to : $this->id; - $this->previousId = $this->previousId === $from ? $to : $this->previousId; + $this->previousIds = array_map(fn (string $id) => $id === $from ? $to : $id, $this->previousIds); $this->storedId = $to; } @@ -986,6 +1048,7 @@ public function flush(): void $this->data = []; $this->replace = true; + $this->ending = true; } /** @@ -998,8 +1061,8 @@ public function regenerate(bool $deleteOldSession = true): void { $this->load(); - if ($deleteOldSession && $this->storedId !== null) { - $this->previousId ??= $this->storedId; + if ($deleteOldSession) { + $this->retire($this->storedId); } $this->id = self::newId(); @@ -1049,13 +1112,15 @@ public function setId(string $id): void // Switching ids must not leave the old session valid. if ($this->storedId !== null && $this->storedId !== $id) { - $this->previousId ??= $this->storedId; + $this->retire($this->storedId); } if (!$store->isClientSide() && !$this->isStateless() && ($record = $this->readFollowing($store, $id)) !== null) { $this->data = $this->original = $record['data']; $this->storedId = $record['id']; $this->snapshot = serialize($this->data); + + $id = $record['id']; } $this->id = $id; @@ -1070,10 +1135,11 @@ public function destroy(): void { $this->load(); - $this->previousId ??= $this->storedId; + $this->retire($this->storedId); $this->storedId = $this->id = $this->snapshot = null; $this->original = []; $this->forgetCookie = true; + $this->ending = true; $this->data = []; $this->peek = []; } diff --git a/src/Phaseolies/Utilities/PathMatcher.php b/src/Phaseolies/Utilities/PathMatcher.php new file mode 100644 index 00000000..ba509a2e --- /dev/null +++ b/src/Phaseolies/Utilities/PathMatcher.php @@ -0,0 +1,20 @@ +logged[] = $failure->getMessage(); + } +} + #[AllowMockObjectsWithoutExpectations] final class DispatchSessionTest extends TestCase { @@ -180,4 +194,224 @@ public function write(string $id, array $data): string $this->addToAssertionCount(1); } + + private function requestOn(string $id): Request + { + $request = $this->request(); + $request->cookies = new \Phaseolies\Http\InputBag([self::COOKIE => $id]); + + return $request; + } + + private function seed(array $data = ['user' => 7]): string + { + $id = bin2hex(random_bytes(20)); + $this->store->write($id, $data + ['last_regenerated' => time()]); + + return $id; + } + + /** + * An application that remembers what it was asked to log. + */ + private function loggingApplication(): LoggingApplication + { + return (new ReflectionClass(LoggingApplication::class))->newInstanceWithoutConstructor(); + } + + public function testALogoutIsAppliedEvenWhenTheRequestThenThrows(): void + { + // The real logout path: forget the user, invalidate, and then something + // else in the request (a listener, a database write) throws. + $id = $this->seed(); + $request = $this->requestOn($id); + + $this->routeTo(function () use ($request) { + $request->session()->forget('user'); + $request->session()->invalidate(); + + throw new RuntimeException('boom'); + }); + + try { + $this->app->dispatch($request); + $this->fail('The exception must still propagate.'); + } catch (RuntimeException $e) { + $this->assertSame('boom', $e->getMessage()); + } + + $this->assertNull($this->store->read($id), 'The logout was dropped: the old session still works.'); + } + + public function testDestroyingTheSessionIsAppliedEvenWhenTheRequestThenThrows(): void + { + $id = $this->seed(); + $request = $this->requestOn($id); + + $this->routeTo(function () use ($request) { + $request->session()->destroy(); + + throw new RuntimeException('boom'); + }); + + try { + $this->app->dispatch($request); + } catch (RuntimeException) { + } + + $this->assertNull($this->store->read($id)); + } + + public function testFlushingTheSessionIsAppliedEvenWhenTheRequestThenThrows(): void + { + $id = $this->seed(); + $request = $this->requestOn($id); + + $this->routeTo(function () use ($request) { + $request->session()->flush(); + $request->session()->put('flash', 'ignored'); + + throw new RuntimeException('boom'); + }); + + try { + $this->app->dispatch($request); + } catch (RuntimeException) { + } + + $this->assertNull($this->store->read($id)); + } + + public function testOrdinaryChangesAreStillDroppedWhenTheRequestThrows(): void + { + $id = $this->seed(); + $request = $this->requestOn($id); + + $this->routeTo(function () use ($request) { + $request->session()->put('cart', ['apple']); + $request->session()->forget('user'); + + throw new RuntimeException('boom'); + }); + + try { + $this->app->dispatch($request); + } catch (RuntimeException) { + } + + $stored = $this->store->read($id)['data']; + + $this->assertSame(7, $stored['user'], 'A plain write was saved by a failed request.'); + $this->assertArrayNotHasKey('cart', $stored); + } + + public function testALoginThatIsInterruptedDoesNotEndTheSessionItWasMoving(): void + { + // regenerate() moves a session to a new id. If the request then throws, + // the old session must stay valid: only an explicit end is applied. + $id = $this->seed(); + $request = $this->requestOn($id); + + $this->routeTo(function () use ($request) { + $request->session()->regenerate(); + $request->session()->put('user', 8); + + throw new RuntimeException('boom'); + }); + + try { + $this->app->dispatch($request); + } catch (RuntimeException) { + } + + $this->assertSame(7, $this->store->read($id)['data']['user']); + } + + public function testASessionThatCannotBeSavedIsLoggedAndTheOriginalErrorStillWins(): void + { + $app = $this->loggingApplication(); + + $request = $this->request(); + $request->session = new Session(new class extends ArrayStore { + public function write(string $id, array $data): string + { + throw new RuntimeException('store down'); + } + }, ['cookie' => self::COOKIE, 'lifetime' => 120, 'stateless' => [], 'lottery' => [0, 100]]); + $request->session->attach($request); + + $redirect = new Response('', 302); + $router = $this->createMock(Router::class); + $router->method('resolve')->willReturnCallback(function () use ($request, $redirect) { + $request->session()->put('a', 1); + + throw (new HttpResponseException('invalid', 422))->setResponse($redirect); + }); + $app->router = $router; + + try { + $app->dispatch($request); + $this->fail('The original exception must propagate.'); + } catch (HttpResponseException) { + } + + $this->assertSame(['store down'], $app->logged, 'The lost save was not reported.'); + } + + public function testASessionChangedBeforeAnHttpErrorIsStillSavedWhenTheErrorPageCannotRender(): void + { + // Rendering the page needs a bootstrapped application, so here it fails. + // The session is saved around it, and no output buffer is left behind. + $id = $this->seed(); + $request = $this->requestOn($id); + + $this->routeTo(function () use ($request) { + $request->session()->put('flash', 'kept'); + + throw new HttpException(403, 'Forbidden'); + }); + + $level = ob_get_level(); + + // In the CLI, PHP warns when a status is set after another test already + // sent one in the same process. That is noise here, not the subject. + set_error_handler(static fn () => true); + + try { + $this->app->dispatch($request); + } catch (\Throwable) { + } finally { + restore_error_handler(); + } + + $this->assertSame($level, ob_get_level(), 'An output buffer was left open.'); + $this->assertSame('kept', $this->store->read($id)['data']['flash'] ?? null); + } + + public function testAChangeMadeWhileAStreamedResponseIsSentIsNotSaved(): void + { + // The session is saved just before the response is sent, because that is + // when its cookie is added, so what a streamed response writes while it + // is produced comes too late. This documents that limit. + $id = $this->seed(); + $request = $this->requestOn($id); + + $this->routeTo(function () use ($request) { + return new \Phaseolies\Http\Response\Stream\StreamedResponse(function () use ($request) { + $request->session()->put('streamed', 'late'); + + echo 'chunk'; + }); + }); + + ob_start(); + + try { + $this->app->dispatch($request); + } finally { + ob_end_clean(); + } + + $this->assertArrayNotHasKey('streamed', $this->store->read($id)['data']); + } } diff --git a/tests/Session/PathMatcherTest.php b/tests/Session/PathMatcherTest.php new file mode 100644 index 00000000..c475f716 --- /dev/null +++ b/tests/Session/PathMatcherTest.php @@ -0,0 +1,99 @@ + ['/api/*', '/api/users', true], + 'no leading slash on the pattern' => ['api/*', '/api/users', true], + 'no leading slash on the path' => ['/api/*', 'api/users', true], + 'deeper path' => ['api/*', '/api/v1/users/1', true], + 'wildcard needs the separator' => ['api/*', '/apix', false], + 'the base itself is not under the wildcard' => ['/api/*', '/api', false], + 'exact' => ['/webhook/payment', '/webhook/payment', true], + 'a trailing slash on the path is significant' => ['/webhook/payment', '/webhook/payment/', false], + 'a trailing slash on the pattern is significant' => ['/webhook/', '/webhook', false], + 'the wildcard matches a bare trailing slash' => ['/api/*', '/api/', true], + 'the wildcard pattern without a leading slash matches it too' => ['api/*', '/api/', true], + 'exact is not a prefix' => ['/webhook', '/webhook/payment', false], + 'case is ignored' => ['/API/*', '/api/Users', true], + 'wildcard in the middle' => ['/api/*/posts', '/api/users/123/posts', true], + 'wildcard in the middle, other tail' => ['/api/*/posts', '/api/users/123/comments', false], + 'root' => ['/', '/', true], + 'root does not match a page' => ['/', '/home', false], + 'everything' => ['*', '/any/thing', true], + 'a dot is a dot' => ['/a.b', '/axb', false], + 'a regex character is literal' => ['/a+b', '/aab', false], + 'a trailing newline does not slip through' => ['/health', "/health\n", false], + ]; + } + + #[DataProvider('cases')] + public function test_the_matcher(string $pattern, string $path, bool $expected): void + { + $this->assertSame($expected, PathMatcher::matches($pattern, $path)); + } + + #[DataProvider('cases')] + public function test_the_session_treats_a_stateless_path_the_same_way(string $pattern, string $path, bool $expected): void + { + $request = new class extends Request { + public string $fakePath = '/'; + + public function __construct() + { + } + + public function getPath(): string + { + return $this->fakePath; + } + }; + $request->fakePath = $path; + $request->cookies = new InputBag([]); + $request->server = new ServerBag([]); + + $session = new Session(new ArrayStore(), ['cookie' => 'test_session', 'stateless' => [$pattern]]); + $session->attach($request); + + $this->assertSame($expected, $session->isStateless()); + } + + #[DataProvider('cases')] + public function test_request_is_treats_the_same_pattern_the_same_way(string $pattern, string $path, bool $expected): void + { + $container = new Container(); + Container::setInstance($container); + + try { + $_SERVER = ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/' . ltrim($path, '/'), 'SERVER_PROTOCOL' => 'HTTP/1.1', 'HTTP_HOST' => 'localhost']; + $request = Request::createFromGlobals(); + $container->singleton('request', fn () => $request); + + // A newline cannot appear in a real request line, so that case is the matcher's alone. + if (!str_contains($path, "\n")) { + $this->assertSame($expected, Request::is($pattern)); + } + } finally { + Container::forgetInstance(); + $_SERVER = []; + } + } +} diff --git a/tests/Session/SessionTest.php b/tests/Session/SessionTest.php index 3fdbb324..9cd2a659 100644 --- a/tests/Session/SessionTest.php +++ b/tests/Session/SessionTest.php @@ -700,6 +700,97 @@ private function sorted(array $values): array return $values; } + public function test_a_request_that_empties_the_session_does_not_delete_a_concurrent_save(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['url.intended' => '/x']); + + // A consumes the only key, so its session looks empty. B logs in and saves first. + $a->forget('url.intended'); + $b->put('user', 7); + $this->commit($b); + + $late = $this->commit($a); + + $this->assertSame(7, $this->stored($cookie)['user'] ?? null, 'The concurrent login was deleted.'); + $this->assertArrayNotHasKey('url.intended', $this->stored($cookie)); + + $cookieSent = $this->sessionCookie($late); + $this->assertNotNull($cookieSent); + $this->assertGreaterThan(time(), $cookieSent->getExpiresTime(), 'The late request expired the cookie.'); + } + + public function test_a_request_that_empties_an_unchanged_session_still_removes_it(): void + { + [$cookie, $a] = $this->twoRequestsOn(['url.intended' => '/x']); + + $a->forget('url.intended'); + $response = $this->commit($a); + + $this->assertNull($this->store()->read($cookie), 'An emptied session was kept.'); + $this->assertSame([], $this->storedFiles()); + $this->assertLessThan(time(), $this->sessionCookie($response)->getExpiresTime()); + } + + public function test_a_request_that_empties_the_session_while_another_creates_a_list_keeps_the_list(): void + { + [$cookie, $a, $b] = $this->twoRequestsOn(['flash' => 'once']); + + // A consumes the flash message, which leaves its session empty. B adds a cart. + $a->forget('flash'); + $b->put('cart', ['apple']); + $this->commit($b); + $this->commit($a); + + $stored = $this->stored($cookie); + + $this->assertArrayNotHasKey('flash', $stored); + $this->assertSame(['apple'], $stored['cart'] ?? null); + } + + public function test_regenerate_after_set_id_ends_both_sessions_it_replaced(): void + { + $first = $this->session(); + $first->put('a', 1); + $a = $this->sessionCookie($this->commit($first))->getValue(); + + $second = $this->session(); + $second->put('b', 2); + $b = $this->sessionCookie($this->commit($second))->getValue(); + + // Load A, switch to B, then regenerate: neither old id may stay valid. + $session = $this->session($this->request('/', [self::COOKIE => $a])); + $session->all(); + $session->setId($b); + $session->regenerate(); + $new = $this->sessionCookie($this->commit($session))->getValue(); + + $this->assertNotContains($new, [$a, $b]); + $this->assertNull($this->store()->read($a), 'The first id is still valid.'); + $this->assertNull($this->store()->read($b), 'The id that was switched to is still valid.'); + $this->assertNotNull($this->store()->read($new)); + } + + public function test_set_id_on_a_rotated_id_uses_the_live_id(): void + { + // X was rotated to Y, so X only holds a pointer. + $y = bin2hex(random_bytes(20)); + $x = bin2hex(random_bytes(20)); + $this->store()->write($y, ['user' => 7, 'last_regenerated' => time()]); + $this->store()->write($x, ['_rotated_to' => $y, '_rotated_until' => time() + 60]); + + $session = $this->session(); + $session->setId($x); + + $this->assertSame($y, $session->getId(), 'getId() returned the pointer instead of the live session.'); + $this->assertSame(7, $session->get('user')); + + $session->put('seen', true); + $cookie = $this->sessionCookie($this->commit($session))->getValue(); + + $this->assertSame($y, $cookie); + $this->assertTrue($this->stored($y)['seen']); + } + public function test_set_id_removes_the_session_it_replaces(): void { $session = $this->session();