[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,7 +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`.
+* [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;
         }
@@ -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.
      *
@@ -188,7 +193,7 @@ public function isUnreadable(): bool
     {
         $this->readData();
 
-        return !$this->stored && $this->store->exists($this->id);
+        return $this->nothingRead && !$this->stored && $this->store->exists($this->id);
     }
 
     public function hydrate(array $attributes): void
@@ -208,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

@@ -54,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;
 

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

@@ -368,6 +368,62 @@ public function testSessionTheStoreHoldsAndReturnsIsNotUnreadable(): void
     }
 
     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
@@ -378,7 +434,7 @@ public function read(Uuid $id): string|false
         $id = Uuid::v4();
         $store->write($id, '{"key":"value"}');
 
-        $this->assertTrue((new Session($store, $id))->isUnreadable());
+        return [$store, $id];
     }
 
     public function testAllReturnsEmptyArrayForNullPayload(): void

tests/Unit/Server/Transport/SessionBindingTest.php

@@ -20,6 +20,7 @@
 use Mcp\Server\Handler\Request\PingHandler;
 use Mcp\Server\Protocol;
 use Mcp\Server\RequestContext;
+use Mcp\Server\Session\FileSessionStore;
 use Mcp\Server\Session\InMemorySessionStore;
 use Mcp\Server\Session\Session;
 use Mcp\Server\Session\SessionManager;
@@ -29,6 +30,7 @@
 use Mcp\Server\Transport\Http\OAuth\AuthorizationTokenValidatorInterface;
 use Mcp\Server\Transport\Http\OAuth\ProtectedResourceMetadata;
 use Mcp\Server\Transport\StreamableHttpTransport;
+use Mcp\Tests\Unit\Server\Session\Fixture\RecordingLogger;
 use Mcp\Tests\Unit\Server\Session\Fixture\RecordingSessionLock;
 use Nyholm\Psr7\Factory\Psr17Factory;
 use PHPUnit\Framework\Attributes\DataProvider;
@@ -37,6 +39,8 @@
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\MiddlewareInterface;
 use Psr\Http\Server\RequestHandlerInterface;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 use Symfony\Component\Uid\Uuid;
 
 /**
@@ -176,6 +180,7 @@ public function testSessionClearedByAHandlerIsNotFound(): void
      */
     public static function provideDataWithoutAttributes(): iterable
     {
+        yield 'an empty string' => [''];
         yield 'empty' => ['[]'];
         yield 'not JSON' => ['{'];
         yield 'not a list of attributes' => ['null'];
@@ -190,6 +195,51 @@ public function testSessionWhoseDataHoldsNoAttributesIsNotFound(string $data): v
         $this->assertSame(404, $this->call($session->toRfc4122(), 'alice.ide.1')->getStatusCode());
     }
 
+    public function testZeroByteSessionFileIsNotFound(): void
+    {
+        $directory = sys_get_temp_dir().'/mcp-session-binding-'.bin2hex(random_bytes(6));
+        $server = Server::builder()
+            ->setServerInfo('test', '1.0.0')
+            ->setSession(new FileSessionStore($directory))
+            ->addTool(self::whoami(...), name: 'whoami')
+            ->build();
+
+        try {
+            $session = $this->server($server, $this->request('POST', $this->initializeRequest(), 'alice.ide.1'))->getHeaderLine('Mcp-Session-Id');
+            file_put_contents($directory.\DIRECTORY_SEPARATOR.$session, '');
+
+            $this->assertSame(404, $this->server($server, $this->request('POST', $this->toolCall(), 'alice.ide.1', $session))->getStatusCode());
+        } finally {
+            array_map('unlink', glob($directory.'/*') ?: []);
+            rmdir($directory);
+        }
+    }
+
+    public function testRefusalOfAnotherPrincipalIsLoggedAsSuch(): void
+    {
+        $logger = new RecordingLogger();
+        $protocol = $this->protocol(logger: $logger);
+        $session = $this->sessionWithQueuedMessage($protocol);
+
+        $this->post($protocol, self::PING, $session, 'carol');
+
+        $this->assertSame(['Refused a request: the session was opened by another principal.'], $logger->messagesAt('warning'));
+    }
+
+    public function testRefusalOfASessionThatRecordsNoPrincipalIsNotLoggedAsAnotherPrincipal(): void
+    {
+        $logger = new RecordingLogger();
+        $protocol = $this->protocol(logger: $logger);
+        $session = Uuid::v4();
+        $this->store->write($session, '{');
+
+        $this->assertSame(404, $this->post($protocol, self::PING, $session, 'alice')->getStatusCode());
+        $this->assertSame(404, $this->endSession($protocol, $session, 'alice')->getStatusCode());
+
+        $reason = 'the session does not record who opened it, so it was cleared, is corrupt or predates the principal binding.';
+        $this->assertSame(['Refused a request: '.$reason, 'Refused a DELETE: '.$reason], $logger->messagesAt('warning'));
+    }
+
     public function testSessionFromBeforeTheUpgradeIsNotFound(): void
     {
         $session = Uuid::v4();
@@ -375,13 +425,14 @@ private function serveWith(ServerRequestInterface $request, array $attributes):
         return $this->serve($request, [new PassthroughMiddleware()]);
     }
 
-    private function protocol(int $gcProbability = 0): Protocol
+    private function protocol(int $gcProbability = 0, LoggerInterface $logger = new NullLogger()): Protocol
     {
         return new Protocol(
             requestHandlers: [new PingHandler()],
             notificationHandlers: [],
             messageFactory: MessageFactory::make(),
             sessionManager: new SessionManager($this->store, gcProbability: $gcProbability, gcDivisor: 1),
+            logger: $logger,
         );
     }
 
@@ -409,6 +460,16 @@ private function post(Protocol $protocol, string $input, Uuid $session, string $
         return $transport->listen();
     }
 
+    private function endSession(Protocol $protocol, Uuid $session, string $principal): ResponseInterface
+    {
+        $request = $this->request('DELETE', '', null, $session->toRfc4122())
+            ->withAttribute(StreamableHttpTransport::PRINCIPAL_ATTRIBUTE, $principal);
+        $transport = new StreamableHttpTransport($request, $this->factory, $this->factory, middleware: [new PassthroughMiddleware()]);
+        $protocol->connect($transport);
+
+        return $transport->listen();
+    }
+
     private function server(Server $server, ServerRequestInterface $request): ResponseInterface
     {
         return $server->run(new StreamableHttpTransport($request, $this->factory, $this->factory, middleware: [$this->authorization()]));