diff --git a/CHANGELOG.md b/CHANGELOG.md index 8db034bd..17e99a5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ All notable changes to `mcp/sdk` will be documented in this file. * [BC Break] `ResourceDefinition::__construct()` signature changed — `$title` parameter added between `$name` and `$description`. Callers using positional arguments must switch to named arguments. * [BC Break] `ResourceTemplate::__construct()` signature changed — `$title` parameter added between `$name` and `$description`. Callers using positional arguments must switch to named arguments. * [BC Break] `McpResource` and `McpResourceTemplate` attribute signatures changed — `$title` parameter added between `$name` and `$description`. Callers using positional arguments must switch to named arguments. +* Fix JSON-RPC error responses fabricating an empty-string `id`. Unrecoverable parse errors (`-32700`) now return `id: null` per JSON-RPC 2.0, and invalid-but-parseable requests (`-32600`) preserve the original request `id`. `Error::$id` / `Error::getId()` and the `$id` parameter of `Error::forParseError()` / `Error::forInvalidRequest()` are widened to `string|int|null`; `InvalidInputMessageException` now carries the recoverable request id via `getRequestId()`/`setRequestId()`. 0.5.0 ----- diff --git a/src/Exception/InvalidInputMessageException.php b/src/Exception/InvalidInputMessageException.php index 4ab485a9..0f961224 100644 --- a/src/Exception/InvalidInputMessageException.php +++ b/src/Exception/InvalidInputMessageException.php @@ -16,4 +16,17 @@ */ class InvalidInputMessageException extends \InvalidArgumentException implements ExceptionInterface { + private string|int|null $requestId = null; + + public function getRequestId(): string|int|null + { + return $this->requestId; + } + + public function setRequestId(string|int|null $requestId): self + { + $this->requestId = $requestId; + + return $this; + } } diff --git a/src/JsonRpc/MessageFactory.php b/src/JsonRpc/MessageFactory.php index 1bb82db8..19705128 100644 --- a/src/JsonRpc/MessageFactory.php +++ b/src/JsonRpc/MessageFactory.php @@ -111,6 +111,11 @@ public function create(string $input): array try { $messages[] = $this->createMessage($message); } catch (InvalidInputMessageException $e) { + // Recover the id only when it's a valid JSON-RPC scalar; + // a null or malformed id is left at the exception's null default. + if (\is_array($message) && isset($message['id']) && (\is_string($message['id']) || \is_int($message['id']))) { + $e->setRequestId($message['id']); + } $messages[] = $e; } } diff --git a/src/Schema/JsonRpc/Error.php b/src/Schema/JsonRpc/Error.php index 532e406d..c5c8d646 100644 --- a/src/Schema/JsonRpc/Error.php +++ b/src/Schema/JsonRpc/Error.php @@ -18,7 +18,7 @@ * * @phpstan-type ErrorData array{ * jsonrpc: string, - * id: string|int, + * id: string|int|null, * code: int, * message: string, * data?: mixed, @@ -42,7 +42,7 @@ class Error implements MessageInterface * @param mixed|null $data additional information about the error */ public function __construct( - public readonly string|int $id, + public readonly string|int|null $id, public readonly int $code, public readonly string $message, public readonly mixed $data = null, @@ -57,10 +57,13 @@ final public static function fromArray(array $data): self if (!isset($data['jsonrpc']) || MessageInterface::JSONRPC_VERSION !== $data['jsonrpc']) { throw new InvalidArgumentException('Invalid or missing "jsonrpc" in Error data.'); } - if (!isset($data['id'])) { + // JSON-RPC requires the "id" key on a response, but its value may be null + // (parse error / invalid request where the id could not be determined), so the + // key must be present yet allowed to hold null alongside string|int. + if (!\array_key_exists('id', $data)) { throw new InvalidArgumentException('Invalid or missing "id" in Error data.'); } - if (!\is_string($data['id']) && !\is_int($data['id'])) { + if (null !== $data['id'] && !\is_string($data['id']) && !\is_int($data['id'])) { throw new InvalidArgumentException('Invalid "id" type in Error data.'); } if (!isset($data['error']) || !\is_array($data['error'])) { @@ -76,42 +79,42 @@ final public static function fromArray(array $data): self return new self($data['id'], $data['error']['code'], $data['error']['message'], $data['error']['data'] ?? null); } - final public static function forParseError(string $message, string|int $id = ''): self + final public static function forParseError(string $message, string|int|null $id = null): self { return new self($id, self::PARSE_ERROR, $message); } - final public static function forInvalidRequest(string $message, string|int $id = ''): self + final public static function forInvalidRequest(string $message, string|int|null $id = null): self { return new self($id, self::INVALID_REQUEST, $message); } - final public static function forMethodNotFound(string $message, string|int $id = ''): self + final public static function forMethodNotFound(string $message, string|int|null $id = null): self { return new self($id, self::METHOD_NOT_FOUND, $message); } - final public static function forInvalidParams(string $message, string|int $id = '', mixed $data = null): self + final public static function forInvalidParams(string $message, string|int|null $id = null, mixed $data = null): self { return new self($id, self::INVALID_PARAMS, $message, $data); } - final public static function forInternalError(string $message, string|int $id = ''): self + final public static function forInternalError(string $message, string|int|null $id = null): self { return new self($id, self::INTERNAL_ERROR, $message); } - final public static function forServerError(string $message, string|int $id = ''): self + final public static function forServerError(string $message, string|int|null $id = null): self { return new self($id, self::SERVER_ERROR, $message); } - final public static function forResourceNotFound(string $message, string|int $id = ''): self + final public static function forResourceNotFound(string $message, string|int|null $id = null): self { return new self($id, self::RESOURCE_NOT_FOUND, $message); } - public function getId(): string|int + public function getId(): string|int|null { return $this->id; } @@ -119,7 +122,7 @@ public function getId(): string|int /** * @return array{ * jsonrpc: string, - * id: string|int, + * id: string|int|null, * error: array{ * code: int, * message: string, diff --git a/src/Server/Protocol.php b/src/Server/Protocol.php index d9af4e4c..9d9f1655 100644 --- a/src/Server/Protocol.php +++ b/src/Server/Protocol.php @@ -151,7 +151,7 @@ private function handleInvalidMessage(TransportInterface $transport, InvalidInpu { $this->logger->warning('Failed to create message.', ['exception' => $exception]); - $error = Error::forInvalidRequest($exception->getMessage()); + $error = Error::forInvalidRequest($exception->getMessage(), $exception->getRequestId()); $this->sendResponse($transport, $error, $session); } diff --git a/tests/Unit/JsonRpc/MessageFactoryTest.php b/tests/Unit/JsonRpc/MessageFactoryTest.php index d38aabeb..719c2923 100644 --- a/tests/Unit/JsonRpc/MessageFactoryTest.php +++ b/tests/Unit/JsonRpc/MessageFactoryTest.php @@ -346,9 +346,23 @@ public function testResponseWithInvalidIdType(): void $this->assertStringContainsString('id', $results[0]->getMessage()); } + public function testErrorWithNullIdIsAccepted(): void + { + // JSON-RPC 2.0 mandates id: null on an error response when the request id + // could not be determined (parse error / invalid request), so an incoming + // error with a null id must deserialize, not be rejected. + $json = '{"jsonrpc": "2.0", "id": null, "error": {"code": -32700, "message": "Parse error"}}'; + + $results = $this->factory->create($json); + + $this->assertCount(1, $results); + $this->assertInstanceOf(Error::class, $results[0]); + $this->assertNull($results[0]->getId()); + } + public function testErrorWithInvalidIdType(): void { - $json = '{"jsonrpc": "2.0", "id": null, "error": {"code": -32600, "message": "Invalid"}}'; + $json = '{"jsonrpc": "2.0", "id": true, "error": {"code": -32600, "message": "Invalid"}}'; $results = $this->factory->create($json); diff --git a/tests/Unit/Server/ProtocolTest.php b/tests/Unit/Server/ProtocolTest.php index f1d1c834..5194b5d9 100644 --- a/tests/Unit/Server/ProtocolTest.php +++ b/tests/Unit/Server/ProtocolTest.php @@ -27,6 +27,7 @@ use Mcp\Server\Session\SessionInterface; use Mcp\Server\Session\SessionManagerInterface; use Mcp\Server\Transport\TransportInterface; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\TestDox; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -297,6 +298,41 @@ public function testInvalidJsonReturnsParseError(): void ); } + #[TestDox('Unrecoverable parse error does not fabricate an empty-string id')] + public function testParseErrorDoesNotFabricateEmptyStringId(): void + { + $sentPayload = null; + $this->transport->expects($this->once()) + ->method('send') + ->willReturnCallback(static function ($data) use (&$sentPayload) { + $sentPayload = $data; + }); + + $protocol = new Protocol( + requestHandlers: [], + notificationHandlers: [], + messageFactory: MessageFactory::make(), + sessionManager: $this->sessionManager, + ); + + // Well-formed JSON nested past PHP's json_decode() depth limit (512), mirroring + // issue #333: json_decode() throws "Maximum stack depth exceeded" so the request + // carries a real numeric id (900512) that cannot be recovered once decoding fails. + $deeplyNested = str_repeat('[', 600).str_repeat(']', 600); + $input = '{"jsonrpc":"2.0","id":900512,"method":"initialize","params":'.$deeplyNested.'}'; + + $protocol->processInput($this->transport, $input, null); + + $this->assertNotNull($sentPayload); + $decoded = json_decode($sentPayload, true); + $this->assertSame(Error::PARSE_ERROR, $decoded['error']['code']); + // The original id is genuinely unrecoverable after a parse failure: per JSON-RPC 2.0 + // the response must emit id: null, never a fabricated empty string and never a dropped + // key (clients that strictly validate the JSON-RPC shape rely on the key being present). + $this->assertArrayHasKey('id', $decoded); + $this->assertNull($decoded['id'], 'Unrecoverable parse error must emit id: null'); + } + #[TestDox('Invalid message structure returns error')] public function testInvalidMessageStructureReturnsError(): void { @@ -349,6 +385,66 @@ public function testInvalidMessageStructureReturnsError(): void $this->assertEquals(Error::INVALID_REQUEST, $message['error']['code']); } + /** + * @return iterable + */ + public static function recoverableIdProvider(): iterable + { + yield 'positive int' => ['{"jsonrpc": "2.0", "id": 42, "params": {}}', 42]; + yield 'zero int (truthiness trap)' => ['{"jsonrpc": "2.0", "id": 0, "params": {}}', 0]; + yield 'string id' => ['{"jsonrpc": "2.0", "id": "req-1", "params": {}}', 'req-1']; + } + + #[DataProvider('recoverableIdProvider')] + #[TestDox('Invalid but parseable message preserves its recoverable id')] + public function testInvalidMessagePreservesRecoverableId(string $input, string|int $expectedId): void + { + $session = $this->createMock(SessionInterface::class); + + $this->sessionManager->method('createWithId')->willReturn($session); + $this->sessionManager->method('exists')->willReturn(true); + + // Configure session mock for queue operations (mirrors testInvalidMessageStructureReturnsError). + $queue = []; + $session->method('get')->willReturnCallback(static function ($key, $default = null) use (&$queue) { + if ('_mcp.outgoing_queue' === $key) { + return $queue; + } + + return $default; + }); + + $session->method('set')->willReturnCallback(static function ($key, $value) use (&$queue) { + if ('_mcp.outgoing_queue' === $key) { + $queue = $value; + } + }); + + $protocol = new Protocol( + requestHandlers: [], + notificationHandlers: [], + messageFactory: MessageFactory::make(), + sessionManager: $this->sessionManager, + ); + + $sessionId = Uuid::v4(); + // Valid JSON carrying a real id but missing method/result/error: the message is + // structurally invalid, yet its id IS recoverable from the decoded payload. + $protocol->processInput( + $this->transport, + $input, + $sessionId + ); + + $outgoing = $protocol->consumeOutgoingMessages($sessionId); + $this->assertCount(1, $outgoing); + + $message = json_decode($outgoing[0]['message'], true); + $this->assertArrayHasKey('error', $message); + $this->assertEquals(Error::INVALID_REQUEST, $message['error']['code']); + $this->assertSame($expectedId, $message['id'], 'Invalid-but-parseable message must preserve its recoverable id, not return ""'); + } + #[TestDox('Request without handler returns method not found error')] public function testRequestWithoutHandlerReturnsMethodNotFoundError(): void {