From a6cc0ddd36ac5d33ee28e0625948c26c67230b30 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Sun, 8 Nov 2026 14:08:39 +0100 Subject: [PATCH 1/2] [Server] Answer 404 for a session that holds no attributes --- CHANGELOG.md | 1 + src/Server/Protocol.php | 4 +- src/Server/Session/Session.php | 7 ++++ src/Server/Session/SessionInterface.php | 8 ++++ tests/Unit/Server/Session/SessionTest.php | 28 ++++++++++++++ .../Server/Transport/SessionBindingTest.php | 38 +++++++++++++++++++ 6 files changed, 84 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 337dd5058..78677d0b7 100644 --- a/CHANGELOG.md +++ b/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` parameters generate `{"type": "object"}` instead of `{"type": "array"}`, and `array` 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`. ### Client diff --git a/src/Server/Protocol.php b/src/Server/Protocol.php index 7c0e46e92..ae0ec20d1 100644 --- a/src/Server/Protocol.php +++ b/src/Server/Protocol.php @@ -1204,7 +1204,7 @@ 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 { @@ -1212,7 +1212,7 @@ private function servesPrincipal(SessionInterface $session, ?string $principal): return true; } - if ([] === $session->all()) { + if ($session->isUnreadable()) { throw new SessionStoreException(\sprintf('Session "%s" could not be read.', $session->getId()->toRfc4122())); } diff --git a/src/Server/Session/Session.php b/src/Server/Session/Session.php index 086398fd4..4de57be5d 100644 --- a/src/Server/Session/Session.php +++ b/src/Server/Session/Session.php @@ -184,6 +184,13 @@ public function all(): array return $this->readData(); } + public function isUnreadable(): bool + { + $this->readData(); + + return !$this->stored && $this->store->exists($this->id); + } + public function hydrate(array $attributes): void { $this->data = $attributes; diff --git a/src/Server/Session/SessionInterface.php b/src/Server/Session/SessionInterface.php index e14d2c637..77a73a597 100644 --- a/src/Server/Session/SessionInterface.php +++ b/src/Server/Session/SessionInterface.php @@ -11,6 +11,7 @@ namespace Mcp\Server\Session; +use Mcp\Exception\SessionStoreException; use Symfony\Component\Uid\Uuid; /** @@ -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. diff --git a/tests/Unit/Server/Session/SessionTest.php b/tests/Unit/Server/Session/SessionTest.php index c09c486ac..d7fba7a25 100644 --- a/tests/Unit/Server/Session/SessionTest.php +++ b/tests/Unit/Server/Session/SessionTest.php @@ -353,6 +353,34 @@ 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 = new class extends InMemorySessionStore { + public function read(Uuid $id): string|false + { + return false; + } + }; + $id = Uuid::v4(); + $store->write($id, '{"key":"value"}'); + + $this->assertTrue((new Session($store, $id))->isUnreadable()); + } + public function testAllReturnsEmptyArrayForNullPayload(): void { $store = $this->getMockBuilder(InMemorySessionStore::class) diff --git a/tests/Unit/Server/Transport/SessionBindingTest.php b/tests/Unit/Server/Transport/SessionBindingTest.php index 6e94cfc58..8f93221c5 100644 --- a/tests/Unit/Server/Transport/SessionBindingTest.php +++ b/tests/Unit/Server/Transport/SessionBindingTest.php @@ -59,6 +59,7 @@ protected function setUp(): void ->setServerInfo('test', '1.0.0') ->setSession($this->store) ->addTool(self::whoami(...), name: 'whoami') + ->addTool(self::forget(...), name: 'forget') ->build(); } @@ -159,6 +160,36 @@ public function testDeleteDuringAStoreOutageKeepsTheSession(): void $this->assertSame('alice', $this->text($this->call($session, 'alice.ide.1'))); } + public function testSessionClearedByAHandlerIsNotFound(): void + { + $session = $this->initialize('alice.ide.1'); + $forget = json_encode(['jsonrpc' => '2.0', 'id' => 2, 'method' => 'tools/call', 'params' => ['name' => 'forget', 'arguments' => new \stdClass()]], \JSON_THROW_ON_ERROR); + + $this->assertSame(200, $this->serve($this->request('POST', $forget, 'alice.ide.1', $session))->getStatusCode()); + + $this->assertSame(404, $this->call($session, 'alice.ide.1')->getStatusCode()); + $this->assertSame(404, $this->serve($this->request('DELETE', '', 'alice.ide.1', $session))->getStatusCode()); + } + + /** + * @return iterable + */ + public static function provideDataWithoutAttributes(): iterable + { + yield 'empty' => ['[]']; + yield 'not JSON' => ['{']; + yield 'not a list of attributes' => ['null']; + } + + #[DataProvider('provideDataWithoutAttributes')] + public function testSessionWhoseDataHoldsNoAttributesIsNotFound(string $data): void + { + $session = Uuid::v4(); + $this->store->write($session, $data); + + $this->assertSame(404, $this->call($session->toRfc4122(), 'alice.ide.1')->getStatusCode()); + } + public function testSessionFromBeforeTheUpgradeIsNotFound(): void { $session = Uuid::v4(); @@ -301,6 +332,13 @@ public function testSessionOpenedWithoutPrincipalOnlyServesRequestsWithoutOne(): $this->assertSame(404, $this->call($session, 'alice.ide.1')->getStatusCode()); } + public static function forget(RequestContext $context): string + { + $context->getSession()->clear(); + + return 'forgotten'; + } + public static function whoami(RequestContext $context): string { return $context->getAccessToken()?->getSubject() ?? 'anonymous'; From 7ccc3a727b3edbcbe9a14e9648be6ee45016d0e2 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Sun, 8 Nov 2026 14:32:38 +0100 Subject: [PATCH 2/2] [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. --- CHANGELOG.md | 2 +- src/Server/Protocol.php | 32 ++++++---- src/Server/Session/FileSessionStore.php | 14 ++++- src/Server/Session/Session.php | 15 ++++- src/Server/Session/SessionInterface.php | 2 +- .../Server/Session/FileSessionStoreTest.php | 17 +++++ tests/Unit/Server/Session/SessionTest.php | 58 ++++++++++++++++- .../Server/Transport/SessionBindingTest.php | 63 ++++++++++++++++++- 8 files changed, 185 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78677d0b7..285e82b09 100644 --- a/CHANGELOG.md +++ b/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` parameters generate `{"type": "object"}` instead of `{"type": "array"}`, and `array` 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 diff --git a/src/Server/Protocol.php b/src/Server/Protocol.php index ae0ec20d1..7959858bc 100644 --- a/src/Server/Protocol.php +++ b/src/Server/Protocol.php @@ -266,7 +266,8 @@ 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); @@ -274,7 +275,7 @@ private function doProcessInput(TransportInterface $transport, string $input, ?U } 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 $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; } diff --git a/src/Server/Session/FileSessionStore.php b/src/Server/Session/FileSessionStore.php index 1f68c3a94..b140a94e8 100644 --- a/src/Server/Session/FileSessionStore.php +++ b/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 diff --git a/src/Server/Session/Session.php b/src/Server/Session/Session.php index 4de57be5d..e5e3855fb 100644 --- a/src/Server/Session/Session.php +++ b/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; } /** diff --git a/src/Server/Session/SessionInterface.php b/src/Server/Session/SessionInterface.php index 77a73a597..5ae93ad76 100644 --- a/src/Server/Session/SessionInterface.php +++ b/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; diff --git a/tests/Unit/Server/Session/FileSessionStoreTest.php b/tests/Unit/Server/Session/FileSessionStoreTest.php index a3a9cf7ec..0a8826527 100644 --- a/tests/Unit/Server/Session/FileSessionStoreTest.php +++ b/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 { diff --git a/tests/Unit/Server/Session/SessionTest.php b/tests/Unit/Server/Session/SessionTest.php index d7fba7a25..d08173a39 100644 --- a/tests/Unit/Server/Session/SessionTest.php +++ b/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 diff --git a/tests/Unit/Server/Transport/SessionBindingTest.php b/tests/Unit/Server/Transport/SessionBindingTest.php index 8f93221c5..dd0a7167d 100644 --- a/tests/Unit/Server/Transport/SessionBindingTest.php +++ b/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()]));