From 9f8a0e9dc4dc94d97a692403faa6c86c7d7befc8 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 1 Oct 2026 00:14:44 +0100 Subject: [PATCH 1/4] fix(session): read session files under a shared lock File::save() writes with file_put_contents(..., LOCK_EX), which truncates the file after taking the lock. File::load() read without a lock, so a concurrent read could see an empty or partial file and return no data. load() now takes a shared lock (flock LOCK_SH) before reading, so it waits for any in-progress write to finish. Co-Authored-By: Claude Opus 5.5 --- src/Session/Storage/File.php | 13 ++--- tests/Session/Storage/FileTest.php | 81 ++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 6 deletions(-) create mode 100644 tests/Session/Storage/FileTest.php diff --git a/src/Session/Storage/File.php b/src/Session/Storage/File.php index 05e3e10b..1ca278a3 100644 --- a/src/Session/Storage/File.php +++ b/src/Session/Storage/File.php @@ -44,14 +44,15 @@ public function save(string $sessionId, array $data): void public function load(string $sessionId): array { - $data = []; $filename = $this->getFilename($sessionId); - if (is_readable($filename)) { - $raw = file_get_contents($filename); - if ($raw !== false) { - $data = json_decode($raw, true); - } + if (! is_readable($filename) || ! ($handle = fopen($filename, 'r'))) { + return []; } + // A shared lock avoids reading a file that save() has truncated but not yet written. + $raw = flock($handle, LOCK_SH) ? stream_get_contents($handle) : false; + flock($handle, LOCK_UN); + fclose($handle); + $data = $raw !== false ? json_decode($raw, true) : []; return is_array($data) ? $data : []; } diff --git a/tests/Session/Storage/FileTest.php b/tests/Session/Storage/FileTest.php new file mode 100644 index 00000000..8fc39fc0 --- /dev/null +++ b/tests/Session/Storage/FileTest.php @@ -0,0 +1,81 @@ +dir = sys_get_temp_dir() . '/platformsh-client-test-' . bin2hex(random_bytes(8)); + } + + protected function tearDown(): void + { + $filename = $this->dir . '/sess-test/sess-test.json'; + if (file_exists($filename)) { + unlink($filename); + } + foreach ([$this->dir . '/sess-test', $this->dir] as $dir) { + if (is_dir($dir)) { + rmdir($dir); + } + } + } + + public function testSaveAndLoad(): void + { + $storage = new File($this->dir); + $this->assertSame([], $storage->load('test')); + $storage->save('test', [ + 'foo' => 'bar', + ]); + $this->assertSame([ + 'foo' => 'bar', + ], $storage->load('test')); + $storage->save('test', []); + $this->assertSame([], $storage->load('test')); + } + + public function testLoadWaitsForExclusiveLock(): void + { + $storage = new File($this->dir); + $storage->save('test', [ + 'token' => 'old', + ]); + $filename = $this->dir . '/sess-test/sess-test.json'; + + // A child process truncates the file under an exclusive lock, as + // file_put_contents() does, and writes the new data after a delay. + $script = <<<'PHP' + $h = fopen($argv[1], 'c'); + flock($h, LOCK_EX); + ftruncate($h, 0); + echo "locked\n"; + usleep(500000); + fwrite($h, '{"token":"new"}'); + fflush($h); + flock($h, LOCK_UN); + fclose($h); + PHP; + $process = proc_open([PHP_BINARY, '-r', $script, $filename], [ + 1 => ['pipe', 'w'], + ], $pipes); + $this->assertIsResource($process); + $this->assertSame("locked\n", fgets($pipes[1])); + + $data = $storage->load('test'); + + fclose($pipes[1]); + $this->assertSame(0, proc_close($process)); + $this->assertSame([ + 'token' => 'new', + ], $data); + } +} From bb696102467ebceb77a0059944597f0ccb12179b Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 1 Oct 2026 00:15:11 +0100 Subject: [PATCH 2/4] feat(session): add Session::reload() and getId() A Session loaded its storage once, so a caller could not see changes saved by another process sharing the same session, such as a rotated refresh token. reload() discards the in-memory data and loads it from storage again. getId() returns the session ID. Both methods are added to SessionInterface, since callers get the session through ConnectorInterface::getSession(). Other implementations of the interface must add them. Co-Authored-By: Claude Opus 5.5 --- src/Session/Session.php | 11 +++++ src/Session/SessionInterface.php | 12 +++++ tests/Session/SessionTest.php | 78 ++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+) create mode 100644 tests/Session/SessionTest.php diff --git a/src/Session/Session.php b/src/Session/Session.php index ba8aaab9..e5f44fb1 100644 --- a/src/Session/Session.php +++ b/src/Session/Session.php @@ -29,6 +29,11 @@ public function __construct(string $id = 'default', array $data = [], SessionSto $this->storage = $storage; } + public function getId(): string + { + return $this->id; + } + public function setStorage(SessionStorageInterface $storage): void { $this->storage = $storage; @@ -69,6 +74,12 @@ public function save(): void $this->storage->save($this->id, $this->data); } + public function reload(): void + { + $this->loaded = false; + $this->lazyLoad(); + } + /** * Load session data, if storage is defined. */ diff --git a/src/Session/SessionInterface.php b/src/Session/SessionInterface.php index c666756e..6e88de29 100644 --- a/src/Session/SessionInterface.php +++ b/src/Session/SessionInterface.php @@ -8,6 +8,11 @@ interface SessionInterface { + /** + * Get the session ID. + */ + public function getId(): string; + /** * Set the storage for this session. */ @@ -30,6 +35,13 @@ public function get(string $key): mixed; */ public function save(); + /** + * Reload the session data from storage, if storage is defined. + * + * Unsaved changes are discarded. + */ + public function reload(); + /** * Clear the session data. */ diff --git a/tests/Session/SessionTest.php b/tests/Session/SessionTest.php new file mode 100644 index 00000000..3a51cab7 --- /dev/null +++ b/tests/Session/SessionTest.php @@ -0,0 +1,78 @@ +storage = new class() implements SessionStorageInterface { + public array $sessions = []; + + public int $saveCount = 0; + + public function load(string $sessionId): array + { + return $this->sessions[$sessionId] ?? []; + } + + public function save(string $sessionId, array $data): void + { + $this->saveCount++; + $this->sessions[$sessionId] = $data; + } + }; + } + + public function testGetId(): void + { + $this->assertSame('default', (new Session())->getId()); + $this->assertSame('foo', (new Session('foo'))->getId()); + } + + public function testReload(): void + { + $session = new Session('test', [], $this->storage); + $this->storage->sessions['test'] = [ + 'token' => 'old', + ]; + $this->assertSame('old', $session->get('token')); + + // Another process changes the stored data. + $this->storage->sessions['test'] = [ + 'token' => 'new', + ]; + $this->assertSame('old', $session->get('token')); + $session->reload(); + $this->assertSame('new', $session->get('token')); + + // Reloaded data is the new baseline for save(). + $session->save(); + $this->assertSame(0, $this->storage->saveCount); + } + + public function testReloadDiscardsUnsavedChanges(): void + { + $session = new Session('test', [], $this->storage); + $session->set('token', 'unsaved'); + $session->reload(); + $this->assertNull($session->get('token')); + } + + public function testReloadWithoutStorage(): void + { + $session = new Session('test', [ + 'token' => 'foo', + ]); + $session->reload(); + $this->assertSame('foo', $session->get('token')); + } +} From 2e2b0cb307a32546696d7814b626f6be4929353a Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 1 Oct 2026 00:15:30 +0100 Subject: [PATCH 3/4] fix(session): update the saved-data snapshot after Session::save() save() skips writing when the data matches the snapshot taken at load time, but the snapshot was not updated after a write. So once the session had changed, every later save() wrote again, even with no further changes. This is costly for storage backends that start a process per write, such as credential helpers. The snapshot is now the JSON encoding of the data, and it is updated after each save. Comparing JSON also detects changes inside JsonSerializable objects, which would share a reference with a plain array copy. Co-Authored-By: Claude Opus 5.5 --- src/Session/Session.php | 21 ++++++++++++++--- tests/Session/SessionTest.php | 43 +++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/Session/Session.php b/src/Session/Session.php index e5f44fb1..e03111cf 100644 --- a/src/Session/Session.php +++ b/src/Session/Session.php @@ -12,7 +12,10 @@ class Session implements SessionInterface private array $data; - private array $original = []; + /** + * The JSON encoding of the data as last loaded or saved. + */ + private ?string $original = null; private bool $loaded = false; @@ -67,11 +70,13 @@ public function save(): void return; } $this->lazyLoad(); - if ($this->data === $this->original) { + $encoded = $this->encode(); + if ($encoded !== null && $encoded === $this->original) { return; } $this->storage->save($this->id, $this->data); + $this->original = $encoded; } public function reload(): void @@ -87,8 +92,18 @@ private function lazyLoad(): void { if (! $this->loaded && isset($this->storage)) { $this->data = $this->storage->load($this->id); - $this->original = $this->data; + $this->original = $this->encode(); $this->loaded = true; } } + + /** + * Encode the data as JSON, so that changes within objects are detected. + */ + private function encode(): ?string + { + $encoded = json_encode($this->data); + + return $encoded === false ? null : $encoded; + } } diff --git a/tests/Session/SessionTest.php b/tests/Session/SessionTest.php index 3a51cab7..a6f732a4 100644 --- a/tests/Session/SessionTest.php +++ b/tests/Session/SessionTest.php @@ -38,6 +38,49 @@ public function testGetId(): void $this->assertSame('foo', (new Session('foo'))->getId()); } + public function testSaveOnlyWritesChanges(): void + { + $session = new Session('test', [], $this->storage); + $session->save(); + $this->assertSame(0, $this->storage->saveCount); + + $session->set('token', 'foo'); + $session->save(); + $this->assertSame(1, $this->storage->saveCount); + $this->assertSame([ + 'token' => 'foo', + ], $this->storage->sessions['test']); + + $session->save(); + $session->set('token', 'foo'); + $session->save(); + $this->assertSame(1, $this->storage->saveCount); + + $session->set('token', 'bar'); + $session->save(); + $this->assertSame(2, $this->storage->saveCount); + } + + public function testSaveWritesChangedObject(): void + { + $value = new class() implements \JsonSerializable { + public string $token = 'foo'; + + public function jsonSerialize(): mixed + { + return $this->token; + } + }; + $session = new Session('test', [], $this->storage); + $session->set('token', $value); + $session->save(); + $this->assertSame(1, $this->storage->saveCount); + + $value->token = 'bar'; + $session->save(); + $this->assertSame(2, $this->storage->saveCount); + } + public function testReload(): void { $session = new Session('test', [], $this->storage); From ee9a6a1dd672b5c9dfe10a5ad41a174ef940b8bb Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 1 Oct 2026 00:32:57 +0100 Subject: [PATCH 4/4] fix(session): read session files even if locking fails flock() can fail on filesystems that don't support locking. load() then returned no data, making a stored login look empty. It now reads without the lock in that case, as it did before. Co-Authored-By: Claude Opus 5.5 --- src/Session/Storage/File.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Session/Storage/File.php b/src/Session/Storage/File.php index 1ca278a3..4df60c47 100644 --- a/src/Session/Storage/File.php +++ b/src/Session/Storage/File.php @@ -49,7 +49,9 @@ public function load(string $sessionId): array return []; } // A shared lock avoids reading a file that save() has truncated but not yet written. - $raw = flock($handle, LOCK_SH) ? stream_get_contents($handle) : false; + // If locking is unsupported, read anyway. + flock($handle, LOCK_SH); + $raw = stream_get_contents($handle); flock($handle, LOCK_UN); fclose($handle); $data = $raw !== false ? json_decode($raw, true) : [];