From 2a24b6f71dca1a9f4b4c7529026c6ab84833997f Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 27 Oct 2026 11:15:16 +0100 Subject: [PATCH 1/2] [Server] Bind Streamable HTTP sessions to the principal that opened them --- CHANGELOG.md | 5 + docs/run/authorization.md | 19 +- src/Server/Protocol.php | 72 +++++- src/Server/Transport/BaseTransport.php | 8 +- .../Middleware/AuthorizationMiddleware.php | 12 +- .../Transport/ManagesTransportCallbacks.php | 4 +- .../Transport/StreamableHttpTransport.php | 27 ++- src/Server/Transport/TransportInterface.php | 6 +- tests/Unit/Server/BuilderTest.php | 5 +- .../Server/Transport/AccessTokenFlowTest.php | 14 +- .../AuthorizationMiddlewareTest.php | 30 +++ .../Server/Transport/SessionBindingTest.php | 227 ++++++++++++++++++ 12 files changed, 396 insertions(+), 33 deletions(-) create mode 100644 tests/Unit/Server/Transport/SessionBindingTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 468c1e9d..e7cc17aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ 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. + 0.9.0 ----- diff --git a/docs/run/authorization.md b/docs/run/authorization.md index 5faa1e84..28697d42 100644 --- a/docs/run/authorization.md +++ b/docs/run/authorization.md @@ -313,16 +313,31 @@ challenge a client steps up from. Use `$token->hasScope()` for decisions that de arguments beyond the tool name. With a `ScopePolicy`, the token's scopes include those implied by its hierarchy. +## Sessions and the Principal + +A handshake-era session only serves the principal that opened it, as the +[security best practices](https://modelcontextprotocol.io/specification/2025-11-25/basic/security_best_practices#session-hijacking) +recommend. `AuthorizationMiddleware` identifies the principal by the issuer, subject and client of +the token. They stay the same when the client refreshes its token, so the session survives the refresh. + +When another principal sends the session's `Mcp-Session-Id`, the server answers 404, as for an +unknown session, and a `DELETE` does not end the session. A token without a `sub` claim identifies +no principal. A session opened with such a token only serves requests without a principal. + ## Authenticating Outside the SDK If your application or framework already authenticates the request, skip `AuthorizationMiddleware` and hand the result to the transport as the PSR-7 request attribute `AccessToken::class`. Both HTTP -transports read it from there, so handlers get it through `RequestContext::getAccessToken()` as usual: +transports read it from there, so handlers get it through `RequestContext::getAccessToken()` as usual. +Also set the `StreamableHttpTransport::PRINCIPAL_ATTRIBUTE` attribute to a string that identifies +the user, so that a session only serves the user that opened it: ```php use Mcp\Server\Authorization\AccessToken; -$request = $request->withAttribute(AccessToken::class, new AccessToken($scopes, $claims)); +$request = $request + ->withAttribute(AccessToken::class, new AccessToken($scopes, $claims)) + ->withAttribute(StreamableHttpTransport::PRINCIPAL_ATTRIBUTE, $userId); $transport = new StreamableHttpTransport($request); ``` diff --git a/src/Server/Protocol.php b/src/Server/Protocol.php index 1d89caca..fba99f44 100644 --- a/src/Server/Protocol.php +++ b/src/Server/Protocol.php @@ -70,6 +70,9 @@ class Protocol public const SESSION_LOGGING_LEVEL = '_mcp.logging_level'; + /** Session key for the fingerprint of the principal that opened the session */ + private const SESSION_PRINCIPAL = '_mcp.principal'; + /** * Deliberately generic: unexpected throwables carry internal details such as file paths, class * names and argument types, which must not be handed to the peer. The full exception is logged. @@ -139,7 +142,7 @@ public function connect(TransportInterface $transport): void { $transport->onMessage($this->processInput(...)); - $transport->onSessionEnd($this->destroySession(...)); + $transport->onSessionEnd($this->endSession(...)); // The transport keeps these callbacks, so they reference it weakly to not keep it alive. $transportRef = \WeakReference::create($transport); @@ -167,13 +170,14 @@ public function connect(TransportInterface $transport): void * * @param TransportInterface $transport * @param AccessToken|null $accessToken the token the input was authorized with, if the transport authorizes + * @param string|null $principal who the input was authenticated as, if the transport authenticates */ - public function processInput(TransportInterface $transport, string $input, ?Uuid $sessionId, ?AccessToken $accessToken = null): void + public function processInput(TransportInterface $transport, string $input, ?Uuid $sessionId, ?AccessToken $accessToken = null, ?string $principal = null): void { // Last line of defense: a malformed message must never escape as a PHP error and take the // server process down. try { - $this->doProcessInput($transport, $input, $sessionId, $accessToken); + $this->doProcessInput($transport, $input, $sessionId, $accessToken, $principal); } catch (\Throwable $e) { $this->logger->error(\sprintf('Uncaught exception while processing input: %s', $e->getMessage()), ['exception' => $e]); @@ -230,7 +234,7 @@ private static function findResponseId(string $input): string|int|null /** * @param TransportInterface $transport */ - private function doProcessInput(TransportInterface $transport, string $input, ?Uuid $sessionId, ?AccessToken $accessToken): void + private function doProcessInput(TransportInterface $transport, string $input, ?Uuid $sessionId, ?AccessToken $accessToken, ?string $principal): void { $this->logger->info('Received message to process.'); $this->logger->debug('Received message payload.', ['message' => $input]); @@ -269,7 +273,7 @@ private function doProcessInput(TransportInterface $transport, string $input, ?U } try { - $this->handleMessages($transport, $sessionId, $messages, $accessToken); + $this->handleMessages($transport, $sessionId, $messages, $accessToken, $principal); } finally { if ($takesLock) { $this->releaseSessionLock($sessionId); @@ -281,9 +285,9 @@ private function doProcessInput(TransportInterface $transport, string $input, ?U * @param TransportInterface $transport * @param array $messages */ - private function handleMessages(TransportInterface $transport, ?Uuid $sessionId, array $messages, ?AccessToken $accessToken): void + private function handleMessages(TransportInterface $transport, ?Uuid $sessionId, array $messages, ?AccessToken $accessToken, ?string $principal): void { - $session = $this->resolveSession($transport, $sessionId, $messages); + $session = $this->resolveSession($transport, $sessionId, $messages, $principal); if (null === $session) { return; } @@ -1112,8 +1116,9 @@ private function hasInitializeRequest(array $messages): bool * @param TransportInterface $transport * @param Uuid|null $sessionId The session ID from the transport * @param array $messages The parsed messages + * @param string|null $principal Who the messages were authenticated as */ - private function resolveSession(TransportInterface $transport, ?Uuid $sessionId, array $messages): ?SessionInterface + private function resolveSession(TransportInterface $transport, ?Uuid $sessionId, array $messages, ?string $principal): ?SessionInterface { if ($this->hasInitializeRequest($messages)) { // Spec: An initialize request must not be part of a batch. @@ -1133,6 +1138,10 @@ private function resolveSession(TransportInterface $transport, ?Uuid $sessionId, } $session = $this->sessionManager->create(); + if (null !== $principal) { + $session->set(self::SESSION_PRINCIPAL, self::fingerprint($principal)); + } + $this->logger->debug('Created new session for initialize', [ 'session_id' => $session->getId()->toRfc4122(), ]); @@ -1171,7 +1180,36 @@ private function resolveSession(TransportInterface $transport, ?Uuid $sessionId, return null; } - return $this->sessionManager->createWithId($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]); + + return null; + } + + return $session; + } + + /** + * Whether the session serves the principal: the one that opened it, or no principal for a session opened without one. + */ + private function servesPrincipal(SessionInterface $session, ?string $principal): bool + { + $fingerprint = $session->get(self::SESSION_PRINCIPAL); + + return (\is_string($fingerprint) ? $fingerprint : null) === self::fingerprint($principal); + } + + private static function fingerprint(?string $principal): ?string + { + return null === $principal ? null : hash('sha256', $principal); } /** @@ -1189,6 +1227,22 @@ private static function findRequestId(array $messages): string|int|null }; } + /** + * Ends a session at the request of a client, which must be the principal that opened it. + */ + private function endSession(Uuid $sessionId, ?string $principal = null): void + { + 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(), + ]); + + return; + } + + $this->destroySession($sessionId); + } + /** * Destroy a specific session. */ diff --git a/src/Server/Transport/BaseTransport.php b/src/Server/Transport/BaseTransport.php index db496f5f..493484c0 100644 --- a/src/Server/Transport/BaseTransport.php +++ b/src/Server/Transport/BaseTransport.php @@ -145,17 +145,17 @@ protected function handleFiberYield(mixed $yielded, ?Uuid $sessionId, \Fiber $fi } } - protected function handleMessage(string $payload, ?Uuid $sessionId, ?AccessToken $accessToken = null): void + protected function handleMessage(string $payload, ?Uuid $sessionId, ?AccessToken $accessToken = null, ?string $principal = null): void { if (\is_callable($this->messageListener)) { - ($this->messageListener)($this, $payload, $sessionId, $accessToken); + ($this->messageListener)($this, $payload, $sessionId, $accessToken, $principal); } } - protected function handleSessionEnd(?Uuid $sessionId): void + protected function handleSessionEnd(?Uuid $sessionId, ?string $principal = null): void { if ($sessionId && \is_callable($this->sessionEndListener)) { - ($this->sessionEndListener)($sessionId); + ($this->sessionEndListener)($sessionId, $principal); } } } diff --git a/src/Server/Transport/Http/Middleware/AuthorizationMiddleware.php b/src/Server/Transport/Http/Middleware/AuthorizationMiddleware.php index 01c7d44e..1c6bf235 100644 --- a/src/Server/Transport/Http/Middleware/AuthorizationMiddleware.php +++ b/src/Server/Transport/Http/Middleware/AuthorizationMiddleware.php @@ -35,6 +35,8 @@ * - Returns 403 insufficient_scope when the token lacks a scope the {@see ScopePolicy} requires * - Hands the validated {@see AccessToken} to the transport as request attribute, * from where handlers read it via {@see \Mcp\Server\RequestContext::getAccessToken()} + * - Identifies the principal by the token's issuer, subject and client, so that a session + * only serves the principal that opened it, see {@see StreamableHttpTransport::PRINCIPAL_ATTRIBUTE} * * @see https://modelcontextprotocol.io/specification/2026-07-28/basic/authorization * @@ -113,7 +115,15 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $token = new AccessToken($granted, $token->getClaims()); } - return $handler->handle($request->withAttribute(AccessToken::class, $token)); + $request = $request->withAttribute(AccessToken::class, $token); + + if (null !== $subject = $token->getSubject()) { + // Kept across token refreshes, unlike the token itself or its scopes. + $principal = json_encode([$token->getClaim('iss'), $subject, $token->getClientId()], \JSON_THROW_ON_ERROR); + $request = $request->withAttribute(StreamableHttpTransport::PRINCIPAL_ATTRIBUTE, $principal); + } + + return $handler->handle($request); } private function buildErrorResponse(AuthorizationResult $result): ResponseInterface diff --git a/src/Server/Transport/ManagesTransportCallbacks.php b/src/Server/Transport/ManagesTransportCallbacks.php index 39bb7255..7084dd2a 100644 --- a/src/Server/Transport/ManagesTransportCallbacks.php +++ b/src/Server/Transport/ManagesTransportCallbacks.php @@ -28,10 +28,10 @@ * */ trait ManagesTransportCallbacks { - /** @var callable(TransportInterface, string, ?Uuid, ?AccessToken): void */ + /** @var callable(TransportInterface, string, ?Uuid, ?AccessToken, ?string): void */ protected $messageListener; - /** @var callable(Uuid): void */ + /** @var callable(Uuid, ?string): void */ protected $sessionEndListener; /** @var callable(Uuid): array}> */ diff --git a/src/Server/Transport/StreamableHttpTransport.php b/src/Server/Transport/StreamableHttpTransport.php index aee34971..51040876 100644 --- a/src/Server/Transport/StreamableHttpTransport.php +++ b/src/Server/Transport/StreamableHttpTransport.php @@ -59,6 +59,14 @@ class StreamableHttpTransport extends BaseTransport implements StatelessAwareTra public const SESSION_HEADER = 'Mcp-Session-Id'; public const PROTOCOL_VERSION_HEADER = 'Mcp-Protocol-Version'; + /** + * PSR-7 request attribute holding a string that identifies the authenticated principal. + * + * A session only serves the principal that opened it. {@see Http\Middleware\AuthorizationMiddleware} + * sets it; an application authenticating requests itself sets it instead. + */ + public const PRINCIPAL_ATTRIBUTE = 'mcp.principal'; + /** * Upper bound on the request body read for a POST, guarding against memory * exhaustion from an oversized (or unbounded chunked) payload. @@ -207,10 +215,11 @@ protected function handleOptionsRequest(): ResponseInterface /** * @param string $body the request body, already read and bounded by {@see self::handleRequest()} * @param AccessToken|null $accessToken the token the request was authorized with, see {@see self::accessToken()} + * @param string|null $principal the principal the request was authenticated as, see {@see self::PRINCIPAL_ATTRIBUTE} */ - protected function handlePostRequest(string $body, ?AccessToken $accessToken = null): ResponseInterface + protected function handlePostRequest(string $body, ?AccessToken $accessToken = null, ?string $principal = null): ResponseInterface { - $this->handleMessage($body, $this->sessionId, $accessToken); + $this->handleMessage($body, $this->sessionId, $accessToken, $principal); // Consume the immediate response exactly once, so a transport instance // reused for a later POST does not replay it. @@ -236,13 +245,16 @@ protected function handlePostRequest(string $body, ?AccessToken $accessToken = n return $this->createJsonResponse(); } - protected function handleDeleteRequest(): ResponseInterface + /** + * @param string|null $principal the principal the request was authenticated as, see {@see self::PRINCIPAL_ATTRIBUTE} + */ + protected function handleDeleteRequest(?string $principal = null): ResponseInterface { if (!$this->sessionId) { return $this->createErrorResponse(Error::forInvalidRequest(self::SESSION_HEADER.' header is required.'), 400); } - $this->handleSessionEnd($this->sessionId); + $this->handleSessionEnd($this->sessionId, $principal); return $this->responseFactory->createResponse(200); } @@ -512,9 +524,12 @@ private function handleHandshakeRequest(ServerRequestInterface $request, ?string return $this->createErrorResponse(Error::forInvalidRequest(self::SESSION_HEADER.' header must be a valid UUID.'), 400); } + $principal = $request->getAttribute(self::PRINCIPAL_ATTRIBUTE); + $principal = \is_string($principal) && '' !== $principal ? $principal : null; + return match ($request->getMethod()) { - 'POST' => $this->handlePostRequest($body ?? '', self::accessToken($request)), - 'DELETE' => $this->handleDeleteRequest(), + 'POST' => $this->handlePostRequest($body ?? '', self::accessToken($request), $principal), + 'DELETE' => $this->handleDeleteRequest($principal), default => $this->createErrorResponse(Error::forInvalidRequest('Method Not Allowed'), 405), }; } diff --git a/src/Server/Transport/TransportInterface.php b/src/Server/Transport/TransportInterface.php index 3ce4e542..381cd990 100644 --- a/src/Server/Transport/TransportInterface.php +++ b/src/Server/Transport/TransportInterface.php @@ -73,8 +73,9 @@ public function close(): void; * The transport calls this whenever ANY message arrives, regardless of source. * * The access token is the one the message was authorized with, if the transport authorizes requests. + * The principal identifies who the message was authenticated as, if the transport authenticates requests. * - * @param callable(TransportInterface $transport, string $message, ?Uuid $sessionId, ?AccessToken $accessToken): void $listener + * @param callable(TransportInterface $transport, string $message, ?Uuid $sessionId, ?AccessToken $accessToken, ?string $principal): void $listener */ public function onMessage(callable $listener): void; @@ -82,8 +83,9 @@ public function onMessage(callable $listener): void; * Register a listener for when a session is terminated. * * The transport calls this when a client disconnects or explicitly ends their session. + * The principal identifies who ended it, if the transport authenticates requests. * - * @param callable(Uuid $sessionId): void $listener The callback function to execute when destroying a session + * @param callable(Uuid $sessionId, ?string $principal): void $listener The callback function to execute when destroying a session */ public function onSessionEnd(callable $listener): void; diff --git a/tests/Unit/Server/BuilderTest.php b/tests/Unit/Server/BuilderTest.php index 53c793f0..75c82e61 100644 --- a/tests/Unit/Server/BuilderTest.php +++ b/tests/Unit/Server/BuilderTest.php @@ -222,7 +222,7 @@ public function testSetSessionLockIsWiredIntoTheProtocol(): void '{"jsonrpc":"2.0","id":2,"method":"ping"}', ])); - $this->assertCount(11, $log, implode(', ', $log)); + $this->assertCount(12, $log, implode(', ', $log)); $sessionId = substr($log[0], \strlen('read ')); $this->assertSame([ // initialize: the session is new, nobody else can know it yet @@ -235,7 +235,8 @@ public function testSetSessionLockIsWiredIntoTheProtocol(): void "read $sessionId", "write $sessionId", "release $sessionId", - // the transport ends the session when it is done + // the transport ends the session when it is done, which checks who opened it first + "read $sessionId", "acquire $sessionId", "destroy $sessionId", "release $sessionId", diff --git a/tests/Unit/Server/Transport/AccessTokenFlowTest.php b/tests/Unit/Server/Transport/AccessTokenFlowTest.php index f080c3a1..eb123ce9 100644 --- a/tests/Unit/Server/Transport/AccessTokenFlowTest.php +++ b/tests/Unit/Server/Transport/AccessTokenFlowTest.php @@ -65,10 +65,10 @@ public function testHandshakeEraHandlerSeesTheTokenOfItsRequest(): void $headers = ['Mcp-Session-Id' => $session, 'MCP-Protocol-Version' => ProtocolVersion::V2025_11_25->value]; $this->assertSame('alice', $this->text($this->serve($server, $this->request($call(2), 'alice', $headers)))); - $this->assertSame('bob', $this->text($this->serve($server, $this->request($call(3), 'bob', $headers)))); + $this->assertSame('alice write', $this->text($this->serve($server, $this->request($call(3), 'alice.write', $headers)))); - // Without the authorization middleware, nothing of the earlier requests is left in the session. - $this->assertSame('anonymous', $this->text($this->serve($server, $this->request($call(4), null, $headers), []))); + // Nothing of the earlier requests is left in the session. + $this->assertSame('alice', $this->text($this->serve($server, $this->request($call(4), 'alice', $headers)))); } public function testModernEraHandlerSeesTheToken(): void @@ -102,7 +102,9 @@ public function testStatelessTransportHandsTheTokenOn(): void public static function whoami(RequestContext $context): string { - return $context->getAccessToken()?->getSubject() ?? 'anonymous'; + $token = $context->getAccessToken(); + + return null === $token ? 'anonymous' : implode(' ', [$token->getSubject(), ...$token->getScopes()]); } private function server(): Server @@ -126,7 +128,9 @@ private function authorization(): AuthorizationMiddleware $validator = new class implements AuthorizationTokenValidatorInterface { public function validate(string $accessToken): AuthorizationResult { - return AuthorizationResult::allow(new AccessToken([], ['sub' => $accessToken])); + [$subject, $scope] = explode('.', $accessToken, 2) + [1 => null]; + + return AuthorizationResult::allow(new AccessToken(null === $scope ? [] : [$scope], ['sub' => $subject])); } }; diff --git a/tests/Unit/Server/Transport/Http/Middleware/AuthorizationMiddlewareTest.php b/tests/Unit/Server/Transport/Http/Middleware/AuthorizationMiddlewareTest.php index 4ee15c48..4354c32e 100644 --- a/tests/Unit/Server/Transport/Http/Middleware/AuthorizationMiddlewareTest.php +++ b/tests/Unit/Server/Transport/Http/Middleware/AuthorizationMiddlewareTest.php @@ -17,6 +17,7 @@ use Mcp\Server\Transport\Http\OAuth\AuthorizationTokenValidatorInterface; use Mcp\Server\Transport\Http\OAuth\ProtectedResourceMetadata; use Mcp\Server\Transport\Http\OAuth\ScopePolicy; +use Mcp\Server\Transport\StreamableHttpTransport; use PHPUnit\Framework\Attributes\DataProvider; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -118,6 +119,23 @@ public function testValidTokenReachesHandlerAsAttribute(): void $this->assertSame($token, $handler->request?->getAttribute(AccessToken::class)); } + public function testPrincipalIsTheIssuerSubjectAndClientOfTheToken(): void + { + $principal = $this->principal(['iss' => 'https://auth.example.com', 'sub' => 'user-1', 'azp' => 'ide', 'jti' => '1'], ['mcp:read']); + + $this->assertIsString($principal); + $this->assertSame($principal, $this->principal(['iss' => 'https://auth.example.com', 'sub' => 'user-1', 'azp' => 'ide', 'jti' => '2'], ['mcp:read', 'mcp:write'])); + $this->assertSame($principal, $this->principal(['iss' => 'https://auth.example.com', 'sub' => 'user-1', 'client_id' => 'ide'], ['mcp:read'])); + $this->assertNotSame($principal, $this->principal(['iss' => 'https://auth.example.com', 'sub' => 'user-2', 'azp' => 'ide'], ['mcp:read'])); + $this->assertNotSame($principal, $this->principal(['iss' => 'https://auth.example.com', 'sub' => 'user-1', 'azp' => 'cli'], ['mcp:read'])); + $this->assertNotSame($principal, $this->principal(['iss' => 'https://other.example.com', 'sub' => 'user-1', 'azp' => 'ide'], ['mcp:read'])); + } + + public function testTokenWithoutSubjectSetsNoPrincipal(): void + { + $this->assertNull($this->principal(['iss' => 'https://auth.example.com', 'azp' => 'ide'], ['mcp:read'])); + } + public function testDefaultScopesAreEnforcedWithoutReadingTheBody(): void { $policy = new ScopePolicy(default: ['mcp:read', 'mcp:write']); @@ -239,6 +257,18 @@ private function request(?string $authorization = null, string $body = ''): Serv return null === $authorization ? $request : $request->withHeader('Authorization', $authorization); } + /** + * @param array $claims + * @param list $scopes + */ + private function principal(array $claims, array $scopes): mixed + { + $handler = $this->capturingHandler(); + $this->middleware($this->validator(AuthorizationResult::allow(new AccessToken($scopes, $claims))))->process($this->request('Bearer token'), $handler); + + return $handler->request?->getAttribute(StreamableHttpTransport::PRINCIPAL_ATTRIBUTE); + } + private function toolCall(string $name): string { return json_encode(['jsonrpc' => '2.0', 'id' => 1, 'method' => 'tools/call', 'params' => ['name' => $name, 'arguments' => new \stdClass()]], \JSON_THROW_ON_ERROR); diff --git a/tests/Unit/Server/Transport/SessionBindingTest.php b/tests/Unit/Server/Transport/SessionBindingTest.php new file mode 100644 index 00000000..8a91bfff --- /dev/null +++ b/tests/Unit/Server/Transport/SessionBindingTest.php @@ -0,0 +1,227 @@ +factory = new Psr17Factory(); + $this->server = Server::builder() + ->setServerInfo('test', '1.0.0') + ->addTool(self::whoami(...), name: 'whoami') + ->build(); + } + + public function testSessionServesTheSubjectThatOpenedIt(): void + { + $session = $this->initialize('alice.ide.1'); + + $this->assertSame('alice', $this->text($this->call($session, 'alice.ide.1'))); + } + + public function testRefreshedTokenKeepsTheSession(): void + { + $session = $this->initialize('alice.ide.1'); + + $this->assertSame('alice', $this->text($this->call($session, 'alice.ide.2'))); + } + + public function testAnotherSubjectIsAnsweredLikeAnUnknownSession(): void + { + $session = $this->initialize('alice.ide.1'); + $unknown = $this->call(Uuid::v4()->toRfc4122(), 'carol.ide.1'); + + $response = $this->call($session, 'carol.ide.1'); + + $this->assertSame(404, $unknown->getStatusCode()); + $this->assertSame(404, $response->getStatusCode()); + $this->assertSame((string) $unknown->getBody(), (string) $response->getBody()); + } + + public function testAnotherClientOfTheSameSubjectIsRefused(): void + { + $session = $this->initialize('alice.ide.1'); + + $this->assertSame(404, $this->call($session, 'alice.cli.1')->getStatusCode()); + } + + public function testRequestWithoutPrincipalIsRefused(): void + { + $session = $this->initialize('alice.ide.1'); + + $this->assertSame(404, $this->call($session, null, [new PassthroughMiddleware()])->getStatusCode()); + } + + public function testDeleteOfAnotherPrincipalKeepsTheSession(): void + { + $session = $this->initialize('alice.ide.1'); + + $this->serve($this->request('DELETE', '', 'carol.ide.1', $session)); + $this->assertSame('alice', $this->text($this->call($session, 'alice.ide.1'))); + + $this->serve($this->request('DELETE', '', 'alice.ide.1', $session)); + $this->assertSame(404, $this->call($session, 'alice.ide.1')->getStatusCode()); + } + + public function testPrincipalSetByTheApplicationBindsTheSession(): void + { + $middleware = new class implements MiddlewareInterface { + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface + { + return $handler->handle($request->withAttribute(StreamableHttpTransport::PRINCIPAL_ATTRIBUTE, $request->getHeaderLine('X-User'))); + } + }; + + $initialize = $this->request('POST', $this->initializeRequest(), null)->withHeader('X-User', 'alice'); + $session = $this->serve($initialize, [$middleware])->getHeaderLine('Mcp-Session-Id'); + + $this->assertSame('anonymous', $this->text($this->serve($this->request('POST', $this->toolCall(), null, $session)->withHeader('X-User', 'alice'), [$middleware]))); + $this->assertSame(404, $this->serve($this->request('POST', $this->toolCall(), null, $session)->withHeader('X-User', 'carol'), [$middleware])->getStatusCode()); + } + + public function testSessionOpenedWithoutPrincipalOnlyServesRequestsWithoutOne(): void + { + $passthrough = [new PassthroughMiddleware()]; + $session = $this->serve($this->request('POST', $this->initializeRequest(), null), $passthrough)->getHeaderLine('Mcp-Session-Id'); + + $this->assertSame('anonymous', $this->text($this->call($session, null, $passthrough))); + $this->assertSame(404, $this->call($session, 'alice.ide.1')->getStatusCode()); + } + + public static function whoami(RequestContext $context): string + { + return $context->getAccessToken()?->getSubject() ?? 'anonymous'; + } + + private function initialize(string $token): string + { + $session = $this->serve($this->request('POST', $this->initializeRequest(), $token))->getHeaderLine('Mcp-Session-Id'); + $this->assertNotSame('', $session); + + return $session; + } + + /** + * @param list|null $middleware + */ + private function call(string $session, ?string $token, ?array $middleware = null): ResponseInterface + { + return $this->serve($this->request('POST', $this->toolCall(), $token, $session), $middleware); + } + + /** + * @param list|null $middleware + */ + private function serve(ServerRequestInterface $request, ?array $middleware = null): ResponseInterface + { + return $this->server->run(new StreamableHttpTransport($request, $this->factory, $this->factory, middleware: $middleware ?? [$this->authorization()])); + } + + /** + * Tokens read `..`: tokens differing only by their serial are refreshes of each other. + */ + private function authorization(): AuthorizationMiddleware + { + $validator = new class implements AuthorizationTokenValidatorInterface { + public function validate(string $accessToken): AuthorizationResult + { + [$subject, $client] = explode('.', $accessToken); + + return AuthorizationResult::allow(new AccessToken([], ['iss' => 'http://localhost:8180', 'sub' => $subject, 'azp' => $client, 'jti' => $accessToken])); + } + }; + + return new AuthorizationMiddleware( + $validator, + new ProtectedResourceMetadata('http://localhost/mcp', ['http://localhost:8180']), + responseFactory: $this->factory, + streamFactory: $this->factory, + ); + } + + private function request(string $method, string $body, ?string $token, ?string $session = null): ServerRequestInterface + { + $request = $this->factory->createServerRequest($method, 'http://localhost/mcp') + ->withHeader('Host', 'localhost') + ->withHeader('Content-Type', 'application/json') + ->withHeader('Accept', 'application/json, text/event-stream') + ->withBody($this->factory->createStream($body)); + + if (null !== $token) { + $request = $request->withHeader('Authorization', 'Bearer '.$token); + } + + if (null !== $session) { + $request = $request + ->withHeader('Mcp-Session-Id', $session) + ->withHeader('MCP-Protocol-Version', ProtocolVersion::V2025_11_25->value); + } + + return $request; + } + + private function initializeRequest(): string + { + return json_encode([ + 'jsonrpc' => '2.0', + 'id' => 1, + 'method' => 'initialize', + 'params' => [ + 'protocolVersion' => ProtocolVersion::V2025_11_25->value, + 'capabilities' => new \stdClass(), + 'clientInfo' => ['name' => 'client', 'version' => '1.0.0'], + ], + ], \JSON_THROW_ON_ERROR); + } + + private function toolCall(): string + { + return json_encode([ + 'jsonrpc' => '2.0', + 'id' => 2, + 'method' => 'tools/call', + 'params' => ['name' => 'whoami', 'arguments' => new \stdClass()], + ], \JSON_THROW_ON_ERROR); + } + + private function text(ResponseInterface $response): string + { + $body = json_decode((string) $response->getBody(), true, flags: \JSON_THROW_ON_ERROR); + + return $body['result']['content'][0]['text'] ?? 'no result: '.$response->getBody(); + } +} From 3c8363ab883ba61b54d2ff82dcea60ae5647b7b0 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Wed, 28 Oct 2026 02:39:54 +0100 Subject: [PATCH 2/2] [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. --- CHANGELOG.md | 2 +- src/Server/Protocol.php | 50 ++++++++++++------- tests/Unit/Server/BuilderTest.php | 5 +- tests/Unit/Server/ProtocolSessionLockTest.php | 48 +++++++++++++++--- tests/Unit/Server/ProtocolSessionRaceTest.php | 2 + tests/Unit/Server/ProtocolTest.php | 49 ++++++++++++------ .../Server/Transport/SessionBindingTest.php | 35 ++++++++++++- 7 files changed, 147 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7cc17aa..d9dcf576 100644 --- a/CHANGELOG.md +++ b/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 ----- diff --git a/src/Server/Protocol.php b/src/Server/Protocol.php index fba99f44..f8e303c4 100644 --- a/src/Server/Protocol.php +++ b/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 $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 diff --git a/tests/Unit/Server/BuilderTest.php b/tests/Unit/Server/BuilderTest.php index 75c82e61..4aab96ba 100644 --- a/tests/Unit/Server/BuilderTest.php +++ b/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", diff --git a/tests/Unit/Server/ProtocolSessionLockTest.php b/tests/Unit/Server/ProtocolSessionLockTest.php index b3dbf8a3..11e9923f 100644 --- a/tests/Unit/Server/ProtocolSessionLockTest.php +++ b/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 */ 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 + */ + 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); } diff --git a/tests/Unit/Server/ProtocolSessionRaceTest.php b/tests/Unit/Server/ProtocolSessionRaceTest.php index f6213624..e13912f6 100644 --- a/tests/Unit/Server/ProtocolSessionRaceTest.php +++ b/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()], diff --git a/tests/Unit/Server/ProtocolTest.php b/tests/Unit/Server/ProtocolTest.php index 24d169c6..8e93a03a 100644 --- a/tests/Unit/Server/ProtocolTest.php +++ b/tests/Unit/Server/ProtocolTest.php @@ -617,7 +617,9 @@ public function write(Uuid $id, string $data): bool $store->throws = $throws; $sessionManager = new SessionManager($store, gcProbability: 0); $sessionId = Uuid::v4(); - $sessionManager->createWithId($sessionId)->save(); + $session = $sessionManager->createWithId($sessionId); + $session->set('_mcp.principal', null); + $session->save(); $store->failWrites = true; // Resumed, the fiber would wait for a client response to a request the session never stored. @@ -680,7 +682,9 @@ public function write(Uuid $id, string $data): bool $store->throws = $throws; $sessionManager = new SessionManager($store, gcProbability: 0); $sessionId = Uuid::v4(); - $sessionManager->createWithId($sessionId)->save(); + $session = $sessionManager->createWithId($sessionId); + $session->set('_mcp.principal', null); + $session->save(); $store->failingWrites = 1; $protocol = new Protocol( @@ -719,6 +723,7 @@ public function testSessionWritesAfterResumeAreSaved(): void $sessionManager = new SessionManager(new InMemorySessionStore(), gcProbability: 0); $sessionId = Uuid::v4(); $session = $sessionManager->createWithId($sessionId); + $session->set('_mcp.principal', null); $session->set('dropped', true); $session->save(); @@ -774,7 +779,11 @@ public function testFailingNotificationListenerDoesNotProduceResponse(): void $queue = []; $session->method('get')->willReturnCallback(static function ($key, $default = null) use (&$queue) { - return '_mcp.outgoing_queue' === $key ? $queue : $default; + return match ($key) { + '_mcp.outgoing_queue' => $queue, + '_mcp.principal' => null, + default => $default, + }; }); $session->method('set')->willReturnCallback(static function ($key, $value) use (&$queue) { if ('_mcp.outgoing_queue' === $key) { @@ -1016,6 +1025,7 @@ public function testOutboundRequestFailureIsAnsweredUnderInboundRequestId(): voi ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $sessionId = $session->getId(); $protocol->processInput( @@ -1139,6 +1149,7 @@ public function testConcurrentWorkersSendClientRequestsUnderDistinctIds(): void $store = new InterleavingSessionStore(); $sessionManager = new SessionManager($store, gcProbability: 0); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $sessionId = $session->getId(); @@ -1183,6 +1194,7 @@ public function testStreamStopsPollingTimedOutRequest(): void ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $sessionId = $session->getId(); @@ -1225,6 +1237,7 @@ private function startTwoStreamsWaitingOnClient(): array ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $sessionId = $session->getId(); @@ -1318,7 +1331,9 @@ public function testResponseIsSentNotQueued(): void $sessions = new SessionManager(new InMemorySessionStore(), gcProbability: 0); $sessionId = Uuid::v4(); - $sessions->createWithId($sessionId)->save(); + $session = $sessions->createWithId($sessionId); + $session->set('_mcp.principal', null); + $session->save(); $transport = new RecordingTransport(); @@ -1454,7 +1469,6 @@ public function testRequestEventIsDispatched(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1523,7 +1537,6 @@ public function testRequestEventModificationIsUsed(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1559,7 +1572,6 @@ public function testRequestEventWithNullDispatcher(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1603,7 +1615,6 @@ public function testResponseEventIsDispatched(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1660,7 +1671,6 @@ public function testResponseEventModificationIsUsed(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1710,7 +1720,6 @@ public function testErrorEventIsDispatchedForErrorResult(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1762,7 +1771,6 @@ public function testErrorEventIsDispatchedForInvalidArgument(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1813,7 +1821,6 @@ public function testErrorEventIsDispatchedForGenericException(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $this->sessionManager->method('createWithId')->willReturn($session); $this->sessionManager->method('exists')->willReturn(true); @@ -1860,7 +1867,6 @@ public function testErrorEventIsDispatchedForMethodNotFound(): void $session = $this->createMock(SessionInterface::class); $session->method('getId')->willReturn(Uuid::v4()); - $session->method('get')->willReturn([]); $session->expects($this->once())->method('save'); $session->expects($this->atLeastOnce())->method('set'); @@ -2222,6 +2228,7 @@ public function testLateResponseToTimedOutRequestIsDropped(): void ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->set('_mcp.pending_requests', ['00000000-0000-4000-8000-000000001000' => ['request_id' => '00000000-0000-4000-8000-000000001000', 'timeout' => 5, 'timestamp' => time() - 10]]); $session->save(); @@ -2349,6 +2356,7 @@ public function testRequestAwaitedByLocalFiberIsKeptPastGracePeriod(): void ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $sessionId = $session->getId(); @@ -2435,6 +2443,7 @@ public function testSuspendedFibersDispatchResultUnderTheirOwnRequest(): void ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": 1, "method": "ping"}', $session->getId()); $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": 2, "method": "ping"}', $session->getId()); @@ -2483,6 +2492,7 @@ public function write(Uuid $id, string $data): bool }; $session = (new SessionManager($store))->create(); + $session->set('_mcp.principal', null); $session->save(); $fibers = []; @@ -2554,6 +2564,7 @@ public function testResumedFiberTurnsThrowingResponseListenerIntoInternalError() ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": 1, "method": "ping"}', $session->getId()); @@ -2606,6 +2617,7 @@ public function testResumedFiberSurvivesThrowingErrorListener(): void ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": 1, "method": "ping"}', $session->getId()); @@ -2655,6 +2667,7 @@ public function testResumedFiberTurnsThrowingHandlerIntoInternalError(): void ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": 1, "method": "ping"}', $session->getId()); @@ -2689,6 +2702,7 @@ public function testClientResponseEventIsNotDispatchedForIdLessError(): void ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->save(); $protocol->processInput($this->transport, '{"jsonrpc": "2.0", "id": null, "error": {"code": -32700, "message": "Parse error"}}', $session->getId()); } @@ -2891,6 +2905,7 @@ private function createProtocolWithPendingRequests(array $pending): array ); $session = $sessionManager->create(); + $session->set('_mcp.principal', null); $session->set('_mcp.pending_requests', $pending); $session->save(); @@ -2908,9 +2923,11 @@ private function createSessionWithPendingRequests(array $requestIds): SessionInt } $session = $this->createMock(SessionInterface::class); - $session->method('get')->willReturnCallback( - static fn (string $key, mixed $default = null): mixed => '_mcp.pending_requests' === $key ? $pending : $default - ); + $session->method('get')->willReturnCallback(static fn (string $key, mixed $default = null): mixed => match ($key) { + '_mcp.pending_requests' => $pending, + '_mcp.principal' => null, + default => $default, + }); return $session; } diff --git a/tests/Unit/Server/Transport/SessionBindingTest.php b/tests/Unit/Server/Transport/SessionBindingTest.php index 8a91bfff..23353bdd 100644 --- a/tests/Unit/Server/Transport/SessionBindingTest.php +++ b/tests/Unit/Server/Transport/SessionBindingTest.php @@ -15,6 +15,7 @@ use Mcp\Server; use Mcp\Server\Authorization\AccessToken; use Mcp\Server\RequestContext; +use Mcp\Server\Session\InMemorySessionStore; use Mcp\Server\Transport\Http\Middleware\AuthorizationMiddleware; use Mcp\Server\Transport\Http\Middleware\PassthroughMiddleware; use Mcp\Server\Transport\Http\OAuth\AuthorizationResult; @@ -35,13 +36,16 @@ final class SessionBindingTest extends TestCase { private Psr17Factory $factory; + private UnreadableSessionStore $store; private Server $server; protected function setUp(): void { $this->factory = new Psr17Factory(); + $this->store = new UnreadableSessionStore(); $this->server = Server::builder() ->setServerInfo('test', '1.0.0') + ->setSession($this->store) ->addTool(self::whoami(...), name: 'whoami') ->build(); } @@ -89,14 +93,30 @@ public function testRequestWithoutPrincipalIsRefused(): void public function testDeleteOfAnotherPrincipalKeepsTheSession(): void { $session = $this->initialize('alice.ide.1'); + $unknown = $this->serve($this->request('DELETE', '', 'carol.ide.1', Uuid::v4()->toRfc4122())); - $this->serve($this->request('DELETE', '', 'carol.ide.1', $session)); + $response = $this->serve($this->request('DELETE', '', 'carol.ide.1', $session)); + $this->assertSame($unknown->getStatusCode(), $response->getStatusCode()); + $this->assertSame((string) $unknown->getBody(), (string) $response->getBody()); $this->assertSame('alice', $this->text($this->call($session, 'alice.ide.1'))); $this->serve($this->request('DELETE', '', 'alice.ide.1', $session)); $this->assertSame(404, $this->call($session, 'alice.ide.1')->getStatusCode()); } + public function testSessionThatCannotBeReadServesNoRequestWithoutPrincipal(): void + { + $session = $this->initialize('alice.ide.1'); + $passthrough = [new PassthroughMiddleware()]; + + $this->store->unreadable = true; + $this->assertSame(404, $this->call($session, null, $passthrough)->getStatusCode()); + $this->serve($this->request('DELETE', '', null, $session), $passthrough); + $this->store->unreadable = false; + + $this->assertSame('alice', $this->text($this->call($session, 'alice.ide.1'))); + } + public function testPrincipalSetByTheApplicationBindsTheSession(): void { $middleware = new class implements MiddlewareInterface { @@ -225,3 +245,16 @@ private function text(ResponseInterface $response): string return $body['result']['content'][0]['text'] ?? 'no result: '.$response->getBody(); } } + +/** + * Finds sessions but fails to read them while $unreadable is set, like a cache that is reachable but fails on get(). + */ +final class UnreadableSessionStore extends InMemorySessionStore +{ + public bool $unreadable = false; + + public function read(Uuid $id): string|false + { + return $this->unreadable ? false : parent::read($id); + } +}