[Server] Fix the session binding on unreadable and busy sessions

A session opened without a principal now records it, so a session the
store fails to read serves no one. Who opened a session is checked before
waiting for its lock, so another principal can't tell the session is busy,
and an initialize never takes the lock.

Author: nicolas-grekas

CHANGELOG.md

@@ -5,7 +5,7 @@ All notable changes to `mcp/sdk` will be documented in this file.
 0.10.0
 ------
 
-* [BC Break] Bind handshake-era sessions to the principal that opened them, as the security best practices recommend. A request of another principal with the session's id is answered with `404`, like an unknown session, and its `DELETE` does not end the session. `AuthorizationMiddleware` identifies the principal by the token's issuer, subject and client; an application that authenticates requests itself sets the `StreamableHttpTransport::PRINCIPAL_ATTRIBUTE` request attribute. `Protocol::processInput()`, `BaseTransport::handleMessage()`, `BaseTransport::handleSessionEnd()`, `StreamableHttpTransport::handlePostRequest()` and `StreamableHttpTransport::handleDeleteRequest()` take a `?string $principal` parameter, as do the `TransportInterface::onMessage()` and `onSessionEnd()` listeners as last argument.
+* [BC Break] Bind handshake-era sessions to the principal that opened them, as the security best practices recommend. A request of another principal with the session's id is answered with `404`, like an unknown session, and its `DELETE` does not end the session. Requests to sessions opened before the upgrade are answered with `404` too, so clients open new ones. `AuthorizationMiddleware` identifies the principal by the token's issuer, subject and client; an application that authenticates requests itself sets the `StreamableHttpTransport::PRINCIPAL_ATTRIBUTE` request attribute. `Protocol::processInput()`, `BaseTransport::handleMessage()`, `BaseTransport::handleSessionEnd()`, `StreamableHttpTransport::handlePostRequest()` and `StreamableHttpTransport::handleDeleteRequest()` take a `?string $principal` parameter, as do the `TransportInterface::onMessage()` and `onSessionEnd()` listeners as last argument.
 
 0.9.0
 -----

src/Server/Protocol.php

@@ -70,7 +70,7 @@ class Protocol
 
     public const SESSION_LOGGING_LEVEL = '_mcp.logging_level';
 
-    /** Session key for the fingerprint of the principal that opened the session */
+    /** Session key for the fingerprint of the principal that opened the session, null when it was opened without one */
     private const SESSION_PRINCIPAL = '_mcp.principal';
 
     /**
@@ -252,13 +252,21 @@ private function doProcessInput(TransportInterface $transport, string $input, ?U
         }
 
         // Taken before the session is read and held until it is saved, so no other request of
-        // the session reads a state this one is about to replace. A session created here for
-        // an `initialize` is not known to anyone else yet and needs no lock, and an unknown
-        // session id is refused by resolveSession() without one; the session is looked up
-        // again under the lock, as it may have ended while this request waited.
-        $takesLock = null !== $sessionId && null !== $this->sessionLock && $this->sessionManager->exists($sessionId);
+        // the session reads a state this one is about to replace. An `initialize` never reads an
+        // existing session and an unknown session id is refused by resolveSession(), so neither
+        // needs the lock. The session is looked up again under the lock, as it may have ended
+        // while this request waited.
+        $takesLock = null !== $sessionId && null !== $this->sessionLock && !$this->hasInitializeRequest($messages) && $this->sessionManager->exists($sessionId);
 
         if ($takesLock) {
+            // 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.
+            if (!$this->servesPrincipal($this->sessionManager->createWithId($sessionId), $principal)) {
+                $this->refuseOtherPrincipal($transport, $sessionId);
+
+                return;
+            }
+
             try {
                 $refusal = $this->acquireSessionLock($sessionId) ? null : Error::forServerError(self::SESSION_BUSY_MESSAGE, self::findResponseId($input));
             } catch (SessionLockException) {
@@ -1138,9 +1146,7 @@ private function resolveSession(TransportInterface $transport, ?Uuid $sessionId,
             }
 
             $session = $this->sessionManager->create();
-            if (null !== $principal) {
-                $session->set(self::SESSION_PRINCIPAL, self::fingerprint($principal));
-            }
+            $session->set(self::SESSION_PRINCIPAL, self::fingerprint($principal));
 
             $this->logger->debug('Created new session for initialize', [
                 'session_id' => $session->getId()->toRfc4122(),
@@ -1183,13 +1189,7 @@ private function resolveSession(TransportInterface $transport, ?Uuid $sessionId,
         $session = $this->sessionManager->createWithId($sessionId);
 
         if (!$this->servesPrincipal($session, $principal)) {
-            $this->logger->warning('Refused a request of another principal than the one that opened the session.', [
-                'session_id' => $sessionId->toRfc4122(),
-            ]);
-
-            // Answered like an unknown session, so that another principal cannot tell the session exists.
-            $error = Error::forInvalidRequest('Session not found or has expired.');
-            $this->sendResponse($transport, $error, null, ['status_code' => 404]);
+            $this->refuseOtherPrincipal($transport, $sessionId);
 
             return null;
         }
@@ -1199,12 +1199,26 @@ private function resolveSession(TransportInterface $transport, ?Uuid $sessionId,
 
     /**
      * Whether the session serves the principal: the one that opened it, or no principal for a session opened without one.
+     *
+     * A session that does not record who opened it, for example because the store failed to read it, serves no one.
      */
     private function servesPrincipal(SessionInterface $session, ?string $principal): bool
     {
-        $fingerprint = $session->get(self::SESSION_PRINCIPAL);
+        return $session->get(self::SESSION_PRINCIPAL, false) === self::fingerprint($principal);
+    }
+
+    /**
+     * @param TransportInterface<mixed> $transport
+     */
+    private function refuseOtherPrincipal(TransportInterface $transport, Uuid $sessionId): void
+    {
+        $this->logger->warning('Refused a request of another principal than the one that opened the session.', [
+            'session_id' => $sessionId->toRfc4122(),
+        ]);
 
-        return (\is_string($fingerprint) ? $fingerprint : null) === self::fingerprint($principal);
+        // Answered like an unknown session, so that another principal cannot tell the session exists.
+        $error = Error::forInvalidRequest('Session not found or has expired.');
+        $this->sendResponse($transport, $error, null, ['status_code' => 404]);
     }
 
     private static function fingerprint(?string $principal): ?string

tests/Unit/Server/BuilderTest.php

@@ -222,14 +222,15 @@ public function testSetSessionLockIsWiredIntoTheProtocol(): void
             '{"jsonrpc":"2.0","id":2,"method":"ping"}',
         ]));
 
-        $this->assertCount(12, $log, implode(', ', $log));
+        $this->assertCount(13, $log, implode(', ', $log));
         $sessionId = substr($log[0], \strlen('read '));
         $this->assertSame([
             // initialize: the session is new, nobody else can know it yet
             "read $sessionId",
             "read $sessionId",
             "write $sessionId",
-            // ping: locked around read and save
+            // ping: who opened the session is checked before waiting for the lock, then locked around read and save
+            "read $sessionId",
             "acquire $sessionId",
             "read $sessionId",
             "read $sessionId",

tests/Unit/Server/ProtocolSessionLockTest.php

@@ -31,6 +31,7 @@
 use Mcp\Tests\Unit\Server\Session\Fixture\RecordingLogger;
 use Mcp\Tests\Unit\Server\Session\Fixture\RecordingSessionLock;
 use Mcp\Tests\Unit\Server\Session\Fixture\RecordingSessionStore;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\TestDox;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
@@ -43,6 +44,8 @@
  */
 final class ProtocolSessionLockTest extends TestCase
 {
+    private const INITIALIZE = '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18","capabilities":{},"clientInfo":{"name":"test","version":"1.0"}}}';
+
     /** @var list<string> */
     private array $log = [];
 
@@ -53,7 +56,7 @@ protected function setUp(): void
         $this->store = new RecordingSessionStore($this->log);
     }
 
-    #[TestDox('a request holds the lock from before the session is read until after it is saved')]
+    #[TestDox('a request holds the lock from before the session is read for handling until after it is saved')]
     public function testRequestHoldsTheLockAroundReadAndSave(): void
     {
         $lock = new RecordingSessionLock($this->log);
@@ -64,6 +67,8 @@ public function testRequestHoldsTheLockAroundReadAndSave(): void
         $protocol->processInput($transport, '{"jsonrpc":"2.0","id":1,"method":"ping"}', $sessionId);
 
         $this->assertSame([
+            // only to check who opened the session
+            "read $sessionId",
             "acquire $sessionId",
             "read $sessionId",
             // save() reads again to write back only the keys this request changed
@@ -122,7 +127,8 @@ public function testSessionEndedWhileWaitingForTheLockIsRefused(): void
         $lock = new RecordingSessionLock($this->log);
         $sessionManager = $this->createMock(SessionManagerInterface::class);
         $sessionManager->method('exists')->willReturnOnConsecutiveCalls(true, false);
-        $sessionManager->expects($this->never())->method('createWithId');
+        // Read before the lock to check who opened the session, and not again once it ended.
+        $sessionManager->expects($this->once())->method('createWithId');
         $protocol = new Protocol([new PingHandler()], [], MessageFactory::make(), $sessionManager, sessionLock: $lock);
         $sessionId = Uuid::v4();
         $transport = new RecordingTransport();
@@ -148,8 +154,8 @@ public function testRequestWithoutSessionIdTakesNoLock(): void
         $this->assertSame(400, $transport->sent[0]['context']['status_code']);
     }
 
-    #[TestDox('a request whose session is locked by another request is refused before the session is read')]
-    public function testBusySessionIsRefusedWithoutReadingIt(): void
+    #[TestDox('a request whose session is locked by another request is refused after reading only who opened the session')]
+    public function testBusySessionIsRefusedReadingOnlyWhoOpenedIt(): void
     {
         $lock = new RecordingSessionLock($this->log, acquirable: false);
         $protocol = $this->protocol($lock);
@@ -158,7 +164,7 @@ public function testBusySessionIsRefusedWithoutReadingIt(): void
 
         $protocol->processInput($transport, '{"jsonrpc":"2.0","id":1,"method":"ping"}', $sessionId);
 
-        $this->assertSame([], $this->log);
+        $this->assertSame(["read $sessionId"], $this->log);
         $this->assertCount(1, $transport->sent);
         $this->assertSame(503, $transport->sent[0]['context']['status_code']);
 
@@ -168,6 +174,34 @@ public function testBusySessionIsRefusedWithoutReadingIt(): void
         $this->assertSame('Another request of this session is still being processed.', $error['error']['message']);
     }
 
+    /**
+     * @return iterable<string, array{string}>
+     */
+    public static function provideInputsOfAnotherPrincipal(): iterable
+    {
+        yield 'a request' => ['{"jsonrpc":"2.0","id":1,"method":"ping"}'];
+        yield 'an initialize request' => [self::INITIALIZE];
+    }
+
+    #[DataProvider('provideInputsOfAnotherPrincipal')]
+    #[TestDox('another principal is answered as for an unknown session without waiting for the lock, so it cannot tell the session is busy')]
+    public function testAnotherPrincipalIsRefusedWithoutWaitingForTheLock(string $input): void
+    {
+        $lock = new RecordingSessionLock($this->log);
+        $protocol = $this->protocol($lock);
+        $opening = new RecordingTransport();
+        $protocol->processInput($opening, self::INITIALIZE, null, null, 'alice');
+        $sessionId = $opening->sent[0]['context']['session_id'];
+        $lock->acquirable = false;
+
+        $unknown = new RecordingTransport();
+        $protocol->processInput($unknown, $input, Uuid::v4(), null, 'carol');
+        $transport = new RecordingTransport();
+        $protocol->processInput($transport, $input, $sessionId, null, 'carol');
+
+        $this->assertSame($unknown->sent, $transport->sent);
+    }
+
     #[TestDox('a busy session is logged as a warning, not as an error')]
     public function testBusySessionIsLoggedAsAWarning(): void
     {
@@ -191,7 +225,7 @@ public function testUnavailableLockIsRefusedAndLoggedWithItsCause(): void
 
         $protocol->processInput($transport, '{"jsonrpc":"2.0","id":1,"method":"ping"}', $sessionId);
 
-        $this->assertSame([], $this->log);
+        $this->assertSame(["read $sessionId"], $this->log);
         $this->assertCount(1, $transport->sent);
         $this->assertSame(503, $transport->sent[0]['context']['status_code']);
 
@@ -616,6 +650,8 @@ private function protocol(RecordingSessionLock $lock, LoggerInterface $logger =
     private function existingSession(array $data = []): Uuid
     {
         $session = new Session($this->store, Uuid::v4());
+        // Opened without a principal
+        $session->set('_mcp.principal', null);
         foreach ($data as $key => $value) {
             $session->set($key, $value);
         }

tests/Unit/Server/ProtocolSessionRaceTest.php

@@ -36,6 +36,7 @@ public function testPollingDoesNotOverwriteAConcurrentResponse(): void
         $sessions = new SessionManager($store, gcProbability: 0);
         $sessionId = Uuid::v4();
         $session = $sessions->createWithId($sessionId);
+        $session->set('_mcp.principal', null);
         $session->set('_mcp.pending_requests', ['00000000-0000-4000-8000-000000000007' => ['request_id' => '00000000-0000-4000-8000-000000000007', 'timeout' => 120, 'timestamp' => time()]]);
         $session->save();
 
@@ -62,6 +63,7 @@ public function testPollingATimedOutRequestDoesNotOverwriteAConcurrentResponse()
         $sessions = new SessionManager($store, gcProbability: 0);
         $sessionId = Uuid::v4();
         $session = $sessions->createWithId($sessionId);
+        $session->set('_mcp.principal', null);
         $session->set('_mcp.pending_requests', [
             '00000000-0000-4000-8000-000000000007' => ['request_id' => '00000000-0000-4000-8000-000000000007', 'timeout' => 5, 'timestamp' => time() - 10],
             '00000000-0000-4000-8000-000000000008' => ['request_id' => '00000000-0000-4000-8000-000000000008', 'timeout' => 120, 'timestamp' => time()],