diff --git a/src/Storage/BuiltinBackend.php b/src/Storage/BuiltinBackend.php index 281d810..ff8f935 100644 --- a/src/Storage/BuiltinBackend.php +++ b/src/Storage/BuiltinBackend.php @@ -17,25 +17,92 @@ use Horde\SessionHandler\SerializedSessionPayload; use Horde\SessionHandler\SessionId; use Horde\SessionHandler\SessionStorageBackend; +use RuntimeException; /** - * Minimal compatibility backend that reads from PHP's native session file - * storage. Implements only SessionStorageBackend -- no iteration, metadata, - * locking, or admin operations. + * Storage backend that reads and writes PHP's standard `sess_` session + * files, including support for the `N;[MODE;]/path` form of + * `session.save_path`. + * + * # Interop scope + * + * The on-disk *layout* matches what PHP's native `mod_files` handler would + * produce: `sess_` files under `session.save_path`, optionally split + * across N levels of single-hex-character subdirectories derived from the + * session id. A site can therefore inspect, archive, or migrate sessions + * with the same tooling it would use for `session.save_handler = files`. + * + * # Divergence from PHP's mod_files + * + * Two deliberate differences: + * + * 1. **No per-request `flock(LOCK_EX)`.** PHP's `mod_files` opens the + * session file once per request, holds an exclusive lock from `read()` + * through `write()`, and only releases it when the session closes. + * That serialises concurrent requests for the same session id at the + * cost of carrying a file descriptor across the request. The modern + * `SessionHandler` PHP-callback shape does not thread a lock between + * `read()` and `write()`; doing so cleanly is a separate cross-cutting + * change. This backend therefore drops the per-request lock and + * accepts the same concurrency model the rest of the modern stack + * already lives with: two simultaneous requests for the same session + * id race, last writer wins. + * + * 2. **Atomic writes via tempfile-and-rename, not in-place truncate-and- + * write.** PHP's `mod_files` writes to the same fd it reads through, so + * a concurrent reader without a shared lock can observe a torn or + * zero-byte file. This backend writes to a sibling tempfile, applies + * mode 0600, then `rename()`s into place. Concurrent readers see + * either the old inode or the new one, never a partial state. This is + * a strictly stronger atomicity guarantee than PHP's, at the cost of + * not coordinating with a hypothetical second writer that uses + * `flock(LOCK_EX)`. We accept that trade for the same reason as (1). + * + * # Auto-creation of subdirectories + * + * Mirrors PHP: the backend does **not** create subdirectories on demand. + * Operators using `session.save_path = "N;..."` are expected to provision + * the tree at install time (typically via `find ... -type d` or a distro + * postinst script). A missing intermediate directory raises a + * {@see RuntimeException} with the exact path so the misconfiguration is + * obvious. + * + * # Garbage collection + * + * Not implemented. This backend does not declare + * {@see \Horde\SessionHandler\IterableSessionBackend} or + * {@see \Horde\SessionHandler\SessionMetadataBackend}, so + * {@see \Horde\SessionHandler\SessionHandler::gc()} returns false and PHP's + * probabilistic GC is a no-op. Operators relying on `Builtin` must run + * external cleanup (cron, `systemd-tmpfiles`, the OS `/tmp` cleaner, or + * similar). Adding GC that walks the hashed tree is a future capability + * tracked separately. */ final class BuiltinBackend implements SessionStorageBackend { private const FILE_PREFIX = 'sess_'; + private const FILE_MODE = 0600; + private const TEMP_PREFIX = '.sess_tmp_'; - public function __construct( - private readonly string $path = '', - ) {} + private readonly PhpFilesSavePathSpec $spec; + + /** + * @param string $path A `session.save_path`-style string. Accepts any + * of the three forms `path`, `N;path`, + * `N;MODE;path`. Empty input falls back to + * {@see session_save_path()} or + * {@see sys_get_temp_dir()}. + */ + public function __construct(string $path = '') + { + $this->spec = PhpFilesSavePathSpec::parse(self::resolvePath($path)); + } public function load(SessionId $id): ?SerializedSessionPayload { $file = $this->filePath($id); - if (!file_exists($file)) { + if (!is_file($file)) { return null; } @@ -48,22 +115,56 @@ public function load(SessionId $id): ?SerializedSessionPayload return new SerializedSessionPayload($contents); } - /** - * Persist session data to PHP's default sess_ file layout. - * - * When Horde registers {@see \Horde\SessionHandler\SessionHandler} as - * PHP's save handler, this backend must write the blob itself — PHP no - * longer invokes the native handler directly. - */ public function save(SessionId $id, SerializedSessionPayload $payload, DateTimeImmutable $expiresAt): void { - $file = $this->filePath($id); - $written = @file_put_contents($file, $payload->getData(), LOCK_EX); + $directory = $this->spec->directoryFor($id->id); + + if (!is_dir($directory)) { + throw new RuntimeException(sprintf( + 'Session save directory "%s" does not exist. Provision the ' + . 'configured save_path tree before starting sessions; this ' + . 'backend does not auto-create subdirectories (matching ' + . 'PHP\'s mod_files).', + $directory, + )); + } + + $finalPath = $directory . '/' . self::FILE_PREFIX . $id->id; + $tempPath = @tempnam($directory, self::TEMP_PREFIX); + + if ($tempPath === false) { + throw new RuntimeException(sprintf( + 'Failed to create temporary session file in "%s"', + $directory, + )); + } + $written = @file_put_contents($tempPath, $payload->getData()); if ($written === false) { - throw new \RuntimeException( - sprintf('Failed to write session file "%s"', $file) - ); + @unlink($tempPath); + throw new RuntimeException(sprintf( + 'Failed to write session data to temporary file "%s"', + $tempPath, + )); + } + + // Apply mode before rename so the file is never visible at its + // final path with looser permissions. + if (!@chmod($tempPath, self::FILE_MODE)) { + @unlink($tempPath); + throw new RuntimeException(sprintf( + 'Failed to set mode 0600 on temporary session file "%s"', + $tempPath, + )); + } + + if (!@rename($tempPath, $finalPath)) { + @unlink($tempPath); + throw new RuntimeException(sprintf( + 'Failed to rename temporary session file "%s" to "%s"', + $tempPath, + $finalPath, + )); } } @@ -74,21 +175,28 @@ public function delete(SessionId $id): void private function filePath(SessionId $id): string { - return $this->getPath() . '/' . self::FILE_PREFIX . $id->id; + return $this->spec->directoryFor($id->id) + . '/' . self::FILE_PREFIX . $id->id; } - private function getPath(): string + /** + * Resolve the configured path argument, honouring PHP's fallback + * chain: explicit value, then {@see session_save_path()}, then + * {@see sys_get_temp_dir()}. Returned as-is for the spec parser to + * interpret (so an `N;MODE;path` value passed in via DI configuration + * still goes through the parser). + */ + private static function resolvePath(string $path): string { - if ($this->path !== '') { - return $this->path; + if ($path !== '') { + return $path; } - $path = session_save_path(); - - if ($path === '' || $path === false) { - return sys_get_temp_dir(); + $sessionPath = session_save_path(); + if ($sessionPath !== '' && $sessionPath !== false) { + return $sessionPath; } - return $path; + return sys_get_temp_dir(); } } diff --git a/src/Storage/PhpFilesSavePathSpec.php b/src/Storage/PhpFilesSavePathSpec.php new file mode 100644 index 0000000..57f480c --- /dev/null +++ b/src/Storage/PhpFilesSavePathSpec.php @@ -0,0 +1,205 @@ + {@see MAX_DEPTH}, and an empty path + * field after splitting. + * + * @throws InvalidArgumentException on any malformed input. + */ + public static function parse(string $input): self + { + if ($input === '') { + throw new InvalidArgumentException( + 'session.save_path is empty', + ); + } + + $parts = explode(';', $input); + $count = count($parts); + + if ($count > 3) { + throw new InvalidArgumentException(sprintf( + 'session.save_path "%s" has more than three semicolon-' + . 'separated fields; expected at most "N;MODE;path"', + $input, + )); + } + + // Last field is always the path. + $basePath = (string) array_pop($parts); + if ($basePath === '') { + throw new InvalidArgumentException(sprintf( + 'session.save_path "%s" has no path component', + $input, + )); + } + + $depth = 0; + $mode = self::DEFAULT_MODE; + + if ($parts !== []) { + $depth = self::parseDepth(array_shift($parts), $input); + } + + if ($parts !== []) { + $mode = self::parseMode(array_shift($parts), $input); + } + + return new self($depth, $mode, $basePath); + } + + /** + * Compute the directory path for a session id, applying the configured + * subdirectory depth. + * + * For depth 0 returns {@see basePath}. For depth N returns + * `basePath///.../`. The session id must contain + * at least `depth` characters (PHP enforces this implicitly because + * its session ids are always longer than any reasonable depth). + */ + public function directoryFor(string $sessionId): string + { + if ($this->depth === 0) { + return $this->basePath; + } + + if (strlen($sessionId) < $this->depth) { + throw new InvalidArgumentException(sprintf( + 'Session id "%s" is shorter than the configured save_path ' + . 'depth %d; cannot derive subdirectory path', + $sessionId, + $this->depth, + )); + } + + $dir = $this->basePath; + for ($i = 0; $i < $this->depth; $i++) { + $dir .= '/' . $sessionId[$i]; + } + + return $dir; + } + + private static function parseDepth(string $raw, string $input): int + { + if ($raw === '' || !ctype_digit($raw)) { + throw new InvalidArgumentException(sprintf( + 'session.save_path "%s" has a non-numeric depth "%s"', + $input, + $raw, + )); + } + + $depth = (int) $raw; + if ($depth < 0 || $depth > self::MAX_DEPTH) { + throw new InvalidArgumentException(sprintf( + 'session.save_path "%s" depth %d is outside the supported ' + . 'range 0..%d', + $input, + $depth, + self::MAX_DEPTH, + )); + } + + return $depth; + } + + private static function parseMode(string $raw, string $input): int + { + if ($raw === '') { + throw new InvalidArgumentException(sprintf( + 'session.save_path "%s" has an empty MODE field', + $input, + )); + } + + // PHP parses MODE as octal regardless of leading-zero prefix. + if (!preg_match('/^[0-7]+$/', $raw)) { + throw new InvalidArgumentException(sprintf( + 'session.save_path "%s" has a non-octal MODE "%s"', + $input, + $raw, + )); + } + + $mode = octdec($raw); + if ($mode < 0 || $mode > 07777) { + throw new InvalidArgumentException(sprintf( + 'session.save_path "%s" MODE %s is outside the valid ' + . 'range 0..07777', + $input, + $raw, + )); + } + + return (int) $mode; + } +} diff --git a/test/unit/Storage/BuiltinBackendTest.php b/test/unit/Storage/BuiltinBackendTest.php new file mode 100644 index 0000000..b5bf5b3 --- /dev/null +++ b/test/unit/Storage/BuiltinBackendTest.php @@ -0,0 +1,229 @@ +tempDir = sys_get_temp_dir() . '/horde_builtin_' . bin2hex(random_bytes(4)); + mkdir($this->tempDir, 0o777, true); + $this->expiresAt = new DateTimeImmutable('+1 hour'); + } + + protected function tearDown(): void + { + $this->removeTree($this->tempDir); + } + + private function removeTree(string $path): void + { + if (!file_exists($path)) { + return; + } + if (is_file($path) || is_link($path)) { + @unlink($path); + return; + } + foreach (scandir($path) ?: [] as $entry) { + if ($entry === '.' || $entry === '..') { + continue; + } + $this->removeTree($path . '/' . $entry); + } + @rmdir($path); + } + + private function payload(string $bytes): SerializedSessionPayload + { + return new SerializedSessionPayload($bytes); + } + + // --------------------------------------------------------------- + // Flat layout (depth 0) + // --------------------------------------------------------------- + + #[Test] + public function testSaveWritesSessFileAtFlatPath(): void + { + $backend = new BuiltinBackend($this->tempDir); + $id = new SessionId('abc123'); + + $backend->save($id, $this->payload('hello'), $this->expiresAt); + + $expected = $this->tempDir . '/sess_abc123'; + self::assertFileExists($expected); + self::assertSame('hello', file_get_contents($expected)); + } + + #[Test] + public function testSavedFileHasMode0600(): void + { + $backend = new BuiltinBackend($this->tempDir); + $id = new SessionId('modetest'); + + $backend->save($id, $this->payload('x'), $this->expiresAt); + + $file = $this->tempDir . '/sess_modetest'; + $perms = fileperms($file) & 0o777; + self::assertSame(0o600, $perms); + } + + #[Test] + public function testLoadReadsBackWhatSaveWrote(): void + { + $backend = new BuiltinBackend($this->tempDir); + $id = new SessionId('roundtrip'); + + $backend->save($id, $this->payload('payload-bytes'), $this->expiresAt); + $loaded = $backend->load($id); + + self::assertNotNull($loaded); + self::assertSame('payload-bytes', $loaded->getData()); + } + + #[Test] + public function testLoadReturnsNullForMissingFile(): void + { + $backend = new BuiltinBackend($this->tempDir); + + self::assertNull($backend->load(new SessionId('notthere'))); + } + + #[Test] + public function testLoadReturnsNullForEmptyFile(): void + { + $backend = new BuiltinBackend($this->tempDir); + touch($this->tempDir . '/sess_empty'); + + self::assertNull($backend->load(new SessionId('empty'))); + } + + #[Test] + public function testDeleteUnlinksFile(): void + { + $backend = new BuiltinBackend($this->tempDir); + $id = new SessionId('todelete'); + $backend->save($id, $this->payload('x'), $this->expiresAt); + + $backend->delete($id); + + self::assertFileDoesNotExist($this->tempDir . '/sess_todelete'); + } + + #[Test] + public function testDeleteOfMissingFileIsSilent(): void + { + $backend = new BuiltinBackend($this->tempDir); + + // Must not raise. + $backend->delete(new SessionId('neverexisted')); + self::assertTrue(true); + } + + #[Test] + public function testSaveOverwritesExistingFile(): void + { + $backend = new BuiltinBackend($this->tempDir); + $id = new SessionId('overwrite'); + + $backend->save($id, $this->payload('first'), $this->expiresAt); + $backend->save($id, $this->payload('second'), $this->expiresAt); + + $loaded = $backend->load($id); + self::assertNotNull($loaded); + self::assertSame('second', $loaded->getData()); + } + + #[Test] + public function testSaveDoesNotLeaveTempfileBehind(): void + { + $backend = new BuiltinBackend($this->tempDir); + + $backend->save(new SessionId('cleanup'), $this->payload('x'), $this->expiresAt); + + $stragglers = glob($this->tempDir . '/.sess_tmp_*') ?: []; + self::assertSame([], $stragglers); + } + + // --------------------------------------------------------------- + // Hashed-tree layout (depth > 0) + // --------------------------------------------------------------- + + #[Test] + public function testSaveUsesHashedSubdirectoryForDepth2(): void + { + // Depth 2: id "abcdef" → tempDir/a/b/sess_abcdef + mkdir($this->tempDir . '/a/b', 0o755, true); + $backend = new BuiltinBackend('2;' . $this->tempDir); + $id = new SessionId('abcdef'); + + $backend->save($id, $this->payload('hashed'), $this->expiresAt); + + $expected = $this->tempDir . '/a/b/sess_abcdef'; + self::assertFileExists($expected); + self::assertSame('hashed', file_get_contents($expected)); + } + + #[Test] + public function testLoadFromHashedSubdirectoryRoundtrips(): void + { + mkdir($this->tempDir . '/3/a', 0o755, true); + $backend = new BuiltinBackend('2;' . $this->tempDir); + $id = new SessionId('3a99ff'); + + $backend->save($id, $this->payload('rt'), $this->expiresAt); + $loaded = $backend->load($id); + + self::assertNotNull($loaded); + self::assertSame('rt', $loaded->getData()); + } + + #[Test] + public function testSaveThrowsWhenSubdirectoryMissing(): void + { + // depth=2 but we never create the a/b tree. + $backend = new BuiltinBackend('2;' . $this->tempDir); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessageMatches('/does not exist/'); + $this->expectExceptionMessageMatches('/does not auto-create/'); + + $backend->save(new SessionId('abcdef'), $this->payload('x'), $this->expiresAt); + } + + #[Test] + public function testDepthModeAndPathFormParses(): void + { + // Just confirm the ctor accepts the three-field form. Mode is + // recorded but not consulted today. + $backend = new BuiltinBackend('1;0700;' . $this->tempDir); + mkdir($this->tempDir . '/a', 0o755, true); + + $backend->save(new SessionId('abc'), $this->payload('x'), $this->expiresAt); + + self::assertFileExists($this->tempDir . '/a/sess_abc'); + } +} diff --git a/test/unit/Storage/PhpFilesSavePathSpecTest.php b/test/unit/Storage/PhpFilesSavePathSpecTest.php new file mode 100644 index 0000000..f6c7bf1 --- /dev/null +++ b/test/unit/Storage/PhpFilesSavePathSpecTest.php @@ -0,0 +1,231 @@ +depth); + self::assertSame(PhpFilesSavePathSpec::DEFAULT_MODE, $spec->mode); + self::assertSame('/var/lib/php/sessions', $spec->basePath); + } + + #[Test] + public function testParsesDepthAndPath(): void + { + $spec = PhpFilesSavePathSpec::parse('2;/var/lib/php/sessions'); + + self::assertSame(2, $spec->depth); + self::assertSame(PhpFilesSavePathSpec::DEFAULT_MODE, $spec->mode); + self::assertSame('/var/lib/php/sessions', $spec->basePath); + } + + #[Test] + public function testParsesDepthModeAndPath(): void + { + $spec = PhpFilesSavePathSpec::parse('2;0700;/var/lib/php/sessions'); + + self::assertSame(2, $spec->depth); + self::assertSame(0700, $spec->mode); + self::assertSame('/var/lib/php/sessions', $spec->basePath); + } + + #[Test] + public function testZeroDepthIsValid(): void + { + $spec = PhpFilesSavePathSpec::parse('0;/tmp'); + + self::assertSame(0, $spec->depth); + self::assertSame('/tmp', $spec->basePath); + } + + #[Test] + public function testMaxDepthIsAccepted(): void + { + $input = PhpFilesSavePathSpec::MAX_DEPTH . ';/tmp'; + $spec = PhpFilesSavePathSpec::parse($input); + + self::assertSame(PhpFilesSavePathSpec::MAX_DEPTH, $spec->depth); + } + + #[Test] + public function testRelativePathIsAccepted(): void + { + // PHP itself does not require absolute paths; we don't either. + $spec = PhpFilesSavePathSpec::parse('1;sessions'); + + self::assertSame(1, $spec->depth); + self::assertSame('sessions', $spec->basePath); + } + + #[Test] + public function testTrailingSlashOnPathIsPreserved(): void + { + // Match PHP: no normalisation. + $spec = PhpFilesSavePathSpec::parse('/tmp/'); + + self::assertSame('/tmp/', $spec->basePath); + } + + // ----------------------------------------------------------------- + // parse(): rejections + // ----------------------------------------------------------------- + + #[Test] + public function testRejectsEmptyInput(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/empty/'); + + PhpFilesSavePathSpec::parse(''); + } + + #[Test] + public function testRejectsTrailingSemicolon(): void + { + // "/tmp;" splits to ["/tmp", ""] — last field empty. + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/no path component/'); + + PhpFilesSavePathSpec::parse('/tmp;'); + } + + #[Test] + public function testRejectsFourFields(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/three semicolon-separated/'); + + PhpFilesSavePathSpec::parse('1;0600;extra;/tmp'); + } + + #[Test] + public function testRejectsNonNumericDepth(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/non-numeric depth/'); + + PhpFilesSavePathSpec::parse('two;/tmp'); + } + + #[Test] + public function testRejectsNegativeDepth(): void + { + // ctype_digit rejects "-1", so the message is "non-numeric depth". + $this->expectException(InvalidArgumentException::class); + + PhpFilesSavePathSpec::parse('-1;/tmp'); + } + + #[Test] + public function testRejectsDepthAboveCap(): void + { + $input = (PhpFilesSavePathSpec::MAX_DEPTH + 1) . ';/tmp'; + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/outside the supported range/'); + + PhpFilesSavePathSpec::parse($input); + } + + #[Test] + public function testRejectsEmptyMode(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/empty MODE/'); + + PhpFilesSavePathSpec::parse('1;;/tmp'); + } + + #[Test] + public function testRejectsNonOctalMode(): void + { + // 0o9 is not a valid octal digit. + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/non-octal MODE/'); + + PhpFilesSavePathSpec::parse('1;0900;/tmp'); + } + + #[Test] + public function testRejectsModeAboveOctalRange(): void + { + // 010000 octal = 4096 decimal, above 07777. + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/outside the valid range/'); + + PhpFilesSavePathSpec::parse('1;010000;/tmp'); + } + + // ----------------------------------------------------------------- + // directoryFor() + // ----------------------------------------------------------------- + + #[Test] + public function testDirectoryForFlatLayout(): void + { + $spec = PhpFilesSavePathSpec::parse('/tmp'); + + self::assertSame('/tmp', $spec->directoryFor('abc123def456')); + } + + #[Test] + public function testDirectoryForOneLevel(): void + { + $spec = PhpFilesSavePathSpec::parse('1;/tmp'); + + self::assertSame('/tmp/a', $spec->directoryFor('abc123')); + } + + #[Test] + public function testDirectoryForTwoLevels(): void + { + $spec = PhpFilesSavePathSpec::parse('2;/tmp'); + + self::assertSame('/tmp/a/b', $spec->directoryFor('abc123')); + } + + #[Test] + public function testDirectoryForThreeLevels(): void + { + $spec = PhpFilesSavePathSpec::parse('3;/var/sess'); + + self::assertSame('/var/sess/3/a/b', $spec->directoryFor('3ab9f0')); + } + + #[Test] + public function testDirectoryForRejectsTooShortSessionId(): void + { + $spec = PhpFilesSavePathSpec::parse('5;/tmp'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/shorter than the configured save_path depth/'); + + $spec->directoryFor('abc'); + } +}