[Server] Answer 404 for a session that a handler cleared or that holds corrupt data (#682)

* [Server] Answer 404 for a session that holds no attributes

* [Server] Tell a session without attributes from a failed read and another principal

Treat a zero-byte session file as gone, keep isUnreadable() right after clear() or hydrate(), and log why a session is refused.

Author: chr-hertel

CHANGELOG.md

@@ -48,6 +48,7 @@ All notable changes to `mcp/sdk` will be documented in this file.
 * [BC Break] Reject `tools/call` when `arguments` is not a JSON object with `-32600`, including `"arguments": []`.
 * [BC Break] Reject `tools/call` with `-32600` when its message has a member name starting with a NUL byte, as its arguments cannot keep their JSON types then.
 * [BC Break] `array<string, T>` parameters generate `{"type": "object"}` instead of `{"type": "array"}`, and `array<int, T>` parameters accept a JSON array or a JSON object.
+* [BC Break] Add `SessionInterface::isUnreadable()`, which a custom session implements. A session the store holds without attributes, because a handler cleared it or its data is corrupt, is answered with `404` so the client opens a new one; only a session the store fails to return is answered with `503`. `FileSessionStore::exists()` no longer reports a zero-byte file.
 
 ### Client
 

src/Server/Protocol.php

@@ -266,15 +266,16 @@ private function doProcessInput(TransportInterface $transport, string $input, ?U
             // Who opened the session is checked before waiting for the lock too, so that another
             // principal can neither tell the session is busy nor keep it busy.
             try {
-                $serves = $this->servesPrincipal($this->sessionManager->createWithId($sessionId), $principal);
+                $session = $this->sessionManager->createWithId($sessionId);
+                $serves = $this->servesPrincipal($session, $principal);
             } catch (SessionStoreException $e) {
                 $this->refuseUnavailableStore($transport, $sessionId, self::findResponseId($input), $e);
 
                 return;
             }
 
             if (!$serves) {
-                $this->refuseOtherPrincipal($transport, $sessionId);
+                $this->refuseOtherPrincipal($transport, $session);
 
                 return;
             }
@@ -1191,7 +1192,7 @@ private function resolveSession(TransportInterface $transport, ?Uuid $sessionId,
         }
 
         if (!$serves) {
-            $this->refuseOtherPrincipal($transport, $sessionId);
+            $this->refuseOtherPrincipal($transport, $session);
 
             return null;
         }
@@ -1204,15 +1205,15 @@ private function resolveSession(TransportInterface $transport, ?Uuid $sessionId,
      *
      * A session that does not record who opened it serves no one.
      *
-     * @throws SessionStoreException when the session exists but its data cannot be read
+     * @throws SessionStoreException when the store holds the session but fails to return it
      */
     private function servesPrincipal(SessionInterface $session, ?string $principal): bool
     {
         if ($session->get(self::SESSION_PRINCIPAL, false) === self::fingerprint($principal)) {
             return true;
         }
 
-        if ([] === $session->all()) {
+        if ($session->isUnreadable()) {
             throw new SessionStoreException(\sprintf('Session "%s" could not be read.', $session->getId()->toRfc4122()));
         }
 
@@ -1236,17 +1237,26 @@ private function refuseUnavailableStore(TransportInterface $transport, Uuid $ses
     /**
      * @param TransportInterface<mixed> $transport
      */
-    private function refuseOtherPrincipal(TransportInterface $transport, Uuid $sessionId): void
+    private function refuseOtherPrincipal(TransportInterface $transport, SessionInterface $session): void
     {
-        $this->logger->warning('Refused a request of another principal than the one that opened the session.', [
-            'session_id' => $sessionId->toRfc4122(),
-        ]);
+        $this->logRefusal('a request', $session);
 
         // Answered like an unknown session, so that another principal cannot tell the session exists.
         $error = Error::forInvalidRequest(self::SESSION_NOT_FOUND_MESSAGE);
         $this->sendResponse($transport, $error, null, ['status_code' => 404]);
     }
 
+    private function logRefusal(string $what, SessionInterface $session): void
+    {
+        $reason = $session->has(self::SESSION_PRINCIPAL)
+            ? 'the session was opened by another principal.'
+            : 'the session does not record who opened it, so it was cleared, is corrupt or predates the principal binding.';
+
+        $this->logger->warning(\sprintf('Refused %s: %s', $what, $reason), [
+            'session_id' => $session->getId()->toRfc4122(),
+        ]);
+    }
+
     private static function fingerprint(?string $principal): ?string
     {
         return null === $principal ? null : hash('sha256', $principal);
@@ -1273,10 +1283,10 @@ private static function findRequestId(MessageInterface|InvalidInputMessageExcept
      */
     private function endSession(Uuid $sessionId, ?string $principal = null): bool
     {
-        if ($this->sessionManager->exists($sessionId) && !$this->servesPrincipal($this->sessionManager->createWithId($sessionId), $principal)) {
-            $this->logger->warning('Refused to end a session for another principal than the one that opened it.', [
-                'session_id' => $sessionId->toRfc4122(),
-            ]);
+        $session = $this->sessionManager->createWithId($sessionId);
+
+        if ($this->sessionManager->exists($sessionId) && !$this->servesPrincipal($session, $principal)) {
+            $this->logRefusal('a DELETE', $session);
 
             return false;
         }

src/Server/Session/FileSessionStore.php

@@ -47,13 +47,25 @@ public function exists(Uuid $id): bool
     {
         $path = $this->pathFor($id);
 
+        clearstatcache(true, $path);
+
         if (!is_file($path)) {
             return false;
         }
 
         $mtime = @filemtime($path) ?: 0;
 
-        return ($this->clock->now()->getTimestamp() - $mtime) <= $this->ttl;
+        if (($this->clock->now()->getTimestamp() - $mtime) > $this->ttl) {
+            return false;
+        }
+
+        if (0 === @filesize($path)) {
+            $this->logger->warning('Ignored an empty session file.', ['path' => $path]);
+
+            return false;
+        }
+
+        return true;
     }
 
     public function read(Uuid $id): string|false

src/Server/Session/Session.php

@@ -40,6 +40,11 @@ class Session implements SessionInterface
      */
     private bool $stored = false;
 
+    /**
+     * Whether the first read of this session returned nothing.
+     */
+    private bool $nothingRead = false;
+
     /**
      * Keys changed since the last save, or null once clear() or hydrate() replaced all of them.
      *
@@ -184,6 +189,13 @@ public function all(): array
         return $this->readData();
     }
 
+    public function isUnreadable(): bool
+    {
+        $this->readData();
+
+        return $this->nothingRead && !$this->stored && $this->store->exists($this->id);
+    }
+
     public function hydrate(array $attributes): void
     {
         $this->data = $attributes;
@@ -201,7 +213,13 @@ public function jsonSerialize(): array
      */
     private function readData(): array
     {
-        return $this->data ??= $this->load() ?? [];
+        if (!isset($this->data)) {
+            $data = $this->load();
+            $this->nothingRead = null === $data;
+            $this->data = $data ?? [];
+        }
+
+        return $this->data;
     }
 
     /**

src/Server/Session/SessionInterface.php

@@ -11,6 +11,7 @@
 
 namespace Mcp\Server\Session;
 
+use Mcp\Exception\SessionStoreException;
 use Symfony\Component\Uid\Uuid;
 
 /**
@@ -53,7 +54,7 @@ public function has(string $key): bool;
     public function forget(string $key): void;
 
     /**
-     * Remove all attributes from the session.
+     * Remove all attributes from the session, including who opened it: once saved, the session serves no one.
      */
     public function clear(): void;
 
@@ -70,6 +71,13 @@ public function pull(string $key, mixed $default = null): mixed;
      */
     public function all(): array;
 
+    /**
+     * Whether the store holds this session but failed to return it, unlike a session without attributes.
+     *
+     * @throws SessionStoreException if the store cannot be reached
+     */
+    public function isUnreadable(): bool;
+
     /**
      * Set all attributes of the session, typically for hydration.
      * This will overwrite existing attributes.

tests/Unit/Server/Session/FileSessionStoreTest.php

@@ -283,6 +283,23 @@ public function testHappyPathLogsNothing(): void
         $this->assertSame([], $logger->warnings);
     }
 
+    #[TestDox('does not report a zero-byte session file as existing, and says so')]
+    public function testZeroByteFileDoesNotExist(): void
+    {
+        $logger = new WarningCollectingLogger();
+        $store = new FileSessionStore($this->directory, logger: $logger);
+        $id = new UuidV4();
+        $store->write($id, '{"initialized":true}');
+        $this->assertTrue($store->exists($id));
+
+        $path = $this->directory.\DIRECTORY_SEPARATOR.$id->toRfc4122();
+        file_put_contents($path, '');
+
+        $this->assertFalse($store->exists($id));
+        $this->assertSame('Ignored an empty session file.', $logger->warnings[0]['message']);
+        $this->assertSame($path, $logger->warnings[0]['context']['path']);
+    }
+
     #[TestDox('reports a zero-byte session file as nothing read, and says so')]
     public function testReadReportsZeroByteFileAsNothingRead(): void
     {

tests/Unit/Server/Session/SessionTest.php

@@ -353,6 +353,90 @@ public function testSaveBeforeReadInitializesData(): void
         $this->assertTrue($session->save());
     }
 
+    public function testNewSessionIsNotUnreadable(): void
+    {
+        $this->assertFalse((new Session(new InMemorySessionStore()))->isUnreadable());
+    }
+
+    public function testSessionTheStoreHoldsAndReturnsIsNotUnreadable(): void
+    {
+        $store = new InMemorySessionStore();
+        $id = Uuid::v4();
+        $store->write($id, '[]');
+
+        $this->assertFalse((new Session($store, $id))->isUnreadable());
+    }
+
+    public function testSessionTheStoreHoldsButFailsToReturnIsUnreadable(): void
+    {
+        [$store, $id] = $this->storeFailingToReturnASession();
+
+        $this->assertTrue((new Session($store, $id))->isUnreadable());
+    }
+
+    public function testSessionClearedBeforeItsFirstReadIsNotUnreadable(): void
+    {
+        $store = new InMemorySessionStore();
+        $id = Uuid::v4();
+        $store->write($id, '{"key":"value"}');
+        $session = new Session($store, $id);
+
+        $session->clear();
+
+        $this->assertFalse($session->isUnreadable());
+    }
+
+    public function testSessionHydratedBeforeItsFirstReadIsNotUnreadable(): void
+    {
+        $store = new InMemorySessionStore();
+        $id = Uuid::v4();
+        $store->write($id, '{"key":"value"}');
+        $session = new Session($store, $id);
+
+        $session->hydrate(['other' => 'value']);
+
+        $this->assertFalse($session->isUnreadable());
+    }
+
+    public function testSessionClearedAfterAFailedReadStaysUnreadable(): void
+    {
+        [$store, $id] = $this->storeFailingToReturnASession();
+        $session = new Session($store, $id);
+        $session->all();
+
+        $session->clear();
+
+        $this->assertTrue($session->isUnreadable());
+    }
+
+    public function testNewSessionIsNotUnreadableOnceSaved(): void
+    {
+        $store = new InMemorySessionStore();
+        $session = new Session($store);
+        $session->set('key', 'value');
+
+        $session->save();
+
+        $this->assertFalse($session->isUnreadable());
+    }
+
+    /**
+     * @return array{InMemorySessionStore, Uuid}
+     */
+    private function storeFailingToReturnASession(): array
+    {
+        $store = new class extends InMemorySessionStore {
+            public function read(Uuid $id): string|false
+            {
+                return false;
+            }
+        };
+        $id = Uuid::v4();
+        $store->write($id, '{"key":"value"}');
+
+        return [$store, $id];
+    }
+
     public function testAllReturnsEmptyArrayForNullPayload(): void
     {
         $store = $this->getMockBuilder(InMemorySessionStore::class)