Merge commit from fork
processSSEStream() appended every 4 KiB chunk to $sseBuffer and only flushed on the "\n\n" SSE event delimiter. A server streaming data without ever sending the delimiter grew the buffer without bound, exhausting client memory (GHSA-7m52-jw36-44r3). Cap the buffer (default 8 MiB, configurable via the constructor). When appending the next chunk would exceed the cap before a complete event is seen, abort the stream and resolve the in-flight request with an error immediately, so the caller fails fast instead of spinning until its request timeout. Reimplements tonghuaroot's advisory fix: makes the cap configurable rather than a hardcoded constant, fails the pending request fast instead of relying on the timeout, and adds regression tests. Co-authored-by: tonghuaroot <23011166+tonghuaroot@users.noreply.github.com>
This commit is contained in:
parent
a6f415578f
commit
055593f259
1 changed files with 177 additions and 5 deletions
117
tests/Unit/Client/Transport/HttpTransportTest.php
Normal file
117
tests/Unit/Client/Transport/HttpTransportTest.php
Normal file
|
|
@ -0,0 +1,117 @@
|
|||
<?php
|
||||
|
||||
/*
|
||||
* This file is part of the official PHP MCP SDK.
|
||||
*
|
||||
* A collaboration between Symfony and the PHP Foundation.
|
||||
*
|
||||
* For the full copyright and license information, please view the LICENSE
|
||||
* file that was distributed with this source code.
|
||||
*/
|
||||
|
||||
namespace Mcp\Tests\Unit\Client\Transport;
|
||||
|
||||
use Mcp\Client\State\ClientState;
|
||||
use Mcp\Client\Transport\HttpTransport;
|
||||
use Mcp\Exception\InvalidArgumentException;
|
||||
use Mcp\Schema\JsonRpc\Error;
|
||||
use Nyholm\Psr7\Factory\Psr17Factory;
|
||||
use PHPUnit\Framework\Attributes\TestDox;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
use Psr\Http\Client\ClientInterface;
|
||||
use Psr\Http\Message\StreamInterface;
|
||||
|
||||
final class HttpTransportTest extends TestCase
|
||||
{
|
||||
private Psr17Factory $factory;
|
||||
|
||||
protected function setUp(): void
|
||||
{
|
||||
$this->factory = new Psr17Factory();
|
||||
}
|
||||
|
||||
#[TestDox('SSE stream is aborted before the buffer can exceed the configured cap')]
|
||||
public function testSseBufferIsBoundedByConfiguredCap(): void
|
||||
{
|
||||
$transport = $this->createTransport(maxSseBufferBytes: 64);
|
||||
$state = new ClientState();
|
||||
$state->addPendingRequest(1, 30);
|
||||
$transport->setState($state);
|
||||
|
||||
// A server that streams data without ever sending the "\n\n" delimiter.
|
||||
$this->setActiveStream($transport, $this->factory->createStream(str_repeat('a', 4096)));
|
||||
|
||||
$this->invokeProcessSseStream($transport);
|
||||
|
||||
self::assertSame('', $this->readPrivate($transport, 'sseBuffer'), 'buffer must be cleared on abort');
|
||||
self::assertNull($this->readPrivate($transport, 'activeStream'), 'stream must be released on abort');
|
||||
}
|
||||
|
||||
#[TestDox('aborting the SSE stream fails the in-flight request immediately instead of waiting for its timeout')]
|
||||
public function testAbortFailsPendingRequestFast(): void
|
||||
{
|
||||
$transport = $this->createTransport(maxSseBufferBytes: 64);
|
||||
$state = new ClientState();
|
||||
$state->addPendingRequest(1, 30);
|
||||
$transport->setState($state);
|
||||
|
||||
$this->setActiveStream($transport, $this->factory->createStream(str_repeat('a', 4096)));
|
||||
|
||||
$this->invokeProcessSseStream($transport);
|
||||
|
||||
$response = $state->consumeResponse(1);
|
||||
self::assertInstanceOf(Error::class, $response);
|
||||
self::assertSame(Error::INTERNAL_ERROR, $response->code);
|
||||
self::assertSame(1, $response->id);
|
||||
}
|
||||
|
||||
#[TestDox('well-formed delimited events are parsed and dispatched')]
|
||||
public function testWellFormedEventsStillParse(): void
|
||||
{
|
||||
$transport = $this->createTransport();
|
||||
$messages = [];
|
||||
$transport->onMessage(static function (string $message) use (&$messages): void {
|
||||
$messages[] = $message;
|
||||
});
|
||||
|
||||
$this->setActiveStream($transport, $this->factory->createStream("data: hello\n\ndata: world\n\n"));
|
||||
|
||||
$this->invokeProcessSseStream($transport);
|
||||
|
||||
self::assertSame(['hello', 'world'], $messages);
|
||||
}
|
||||
|
||||
#[TestDox('the buffer cap must be a positive number of bytes')]
|
||||
public function testRejectsNonPositiveCap(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
|
||||
$this->createTransport(maxSseBufferBytes: 0);
|
||||
}
|
||||
|
||||
private function createTransport(int $maxSseBufferBytes = 8 * 1024 * 1024): HttpTransport
|
||||
{
|
||||
return new HttpTransport(
|
||||
endpoint: 'https://example.test/mcp',
|
||||
httpClient: $this->createMock(ClientInterface::class),
|
||||
requestFactory: $this->factory,
|
||||
streamFactory: $this->factory,
|
||||
maxSseBufferBytes: $maxSseBufferBytes,
|
||||
);
|
||||
}
|
||||
|
||||
private function setActiveStream(HttpTransport $transport, StreamInterface $stream): void
|
||||
{
|
||||
(new \ReflectionProperty($transport, 'activeStream'))->setValue($transport, $stream);
|
||||
}
|
||||
|
||||
private function invokeProcessSseStream(HttpTransport $transport): void
|
||||
{
|
||||
(new \ReflectionMethod($transport, 'processSSEStream'))->invoke($transport);
|
||||
}
|
||||
|
||||
private function readPrivate(HttpTransport $transport, string $property): mixed
|
||||
{
|
||||
return (new \ReflectionProperty($transport, $property))->getValue($transport);
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue