From 94d24eea76183bd3a583393d9b475a983a624c1b Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Tue, 5 May 2026 13:43:06 +0200 Subject: [PATCH] feat(IClient): let guzzle choose the handler for http requests so it can write a streamed response body progressively Signed-off-by: Julien Veyssier --- lib/private/Http/Client/Client.php | 16 ++- lib/private/Http/Client/ClientService.php | 6 +- tests/lib/Http/Client/ClientServiceTest.php | 126 ++++++++++++-------- tests/lib/Http/Client/ClientTest.php | 68 +++++++++++ 4 files changed, 161 insertions(+), 55 deletions(-) diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index 97ef67d49f66d..0923883ee6f46 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -42,15 +42,19 @@ public function __construct( } private function buildRequestOptions(array $options): array { + $streamResponse = !empty($options[RequestOptions::STREAM]); $proxy = $this->getProxyUri(); $defaults = [ RequestOptions::VERIFY => $this->getCertBundle(), RequestOptions::TIMEOUT => IClient::DEFAULT_REQUEST_TIMEOUT, - // Prefer HTTP/2 globally (PSR-7 request version) - RequestOptions::VERSION => '2.0', + // Guzzle's StreamHandler only supports HTTP/1.x, so streamed + // responses must not force the default HTTP/2 transport settings. + RequestOptions::VERSION => $streamResponse ? '1.1' : '2.0', ]; - $defaults['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_2TLS; + $defaults['curl'][\CURLOPT_HTTP_VERSION] = $streamResponse + ? \CURL_HTTP_VERSION_1_1 + : \CURL_HTTP_VERSION_2TLS; $options['nextcloud']['allow_local_address'] = $this->isLocalAddressAllowed($options); if ($options['nextcloud']['allow_local_address'] === false) { @@ -76,6 +80,12 @@ private function buildRequestOptions(array $options): array { $options = array_merge($defaults, $options); + if ($streamResponse) { + $options[RequestOptions::VERSION] = '1.1'; + $options['curl'] ??= []; + $options['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_1_1; + } + if (!isset($options[RequestOptions::HEADERS]['User-Agent'])) { $userAgent = 'Nextcloud-Server-Crawler/' . $this->serverVersion->getVersionString(); $overwriteCliUrl = $this->config->getSystemValueString('overwrite.cli.url'); diff --git a/lib/private/Http/Client/ClientService.php b/lib/private/Http/Client/ClientService.php index 8df3d3ed193bc..701976def9371 100644 --- a/lib/private/Http/Client/ClientService.php +++ b/lib/private/Http/Client/ClientService.php @@ -10,9 +10,9 @@ namespace OC\Http\Client; use GuzzleHttp\Client as GuzzleClient; -use GuzzleHttp\Handler\CurlHandler; use GuzzleHttp\HandlerStack; use GuzzleHttp\Middleware; +use GuzzleHttp\Utils; use OCP\Diagnostics\IEventLogger; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; @@ -42,7 +42,9 @@ public function __construct( #[\Override] public function newClient(): IClient { - $handler = new CurlHandler(); + // allows using a StreamHandler if streaming is enabled in the request options + // and allow_url_fopen is enabled in the Php config + $handler = Utils::chooseHandler(); $stack = HandlerStack::create($handler); if ($this->config->getSystemValueBool('dns_pinning', true)) { $stack->push($this->dnsPinMiddleware->addDnsPinning()); diff --git a/tests/lib/Http/Client/ClientServiceTest.php b/tests/lib/Http/Client/ClientServiceTest.php index e7f116b9b6910..833a61859450b 100644 --- a/tests/lib/Http/Client/ClientServiceTest.php +++ b/tests/lib/Http/Client/ClientServiceTest.php @@ -10,10 +10,9 @@ namespace Test\Http\Client; -use GuzzleHttp\Client as GuzzleClient; -use GuzzleHttp\Handler\CurlHandler; +use GuzzleHttp\Handler\MockHandler; use GuzzleHttp\HandlerStack; -use GuzzleHttp\Middleware; +use GuzzleHttp\Psr7\Response; use OC\Http\Client\Client; use OC\Http\Client\ClientService; use OC\Http\Client\DnsPinMiddleware; @@ -22,7 +21,6 @@ use OCP\IConfig; use OCP\Security\IRemoteHostValidator; use OCP\ServerVersion; -use Psr\Http\Message\RequestInterface; use Psr\Log\LoggerInterface; /** @@ -32,21 +30,38 @@ class ClientServiceTest extends \Test\TestCase { public function testNewClient(): void { /** @var IConfig $config */ $config = $this->createMock(IConfig::class); - $config->method('getSystemValueBool') - ->with('dns_pinning', true) - ->willReturn(true); + $config->method('getSystemValueBool')->willReturnMap([ + ['dns_pinning', true, true], + ['installed', false, false], + ['allow_local_remote_servers', false, false], + ['http_client_add_user_agent_url', false, false], + ]); /** @var ICertificateManager $certificateManager */ $certificateManager = $this->createMock(ICertificateManager::class); $dnsPinMiddleware = $this->createMock(DnsPinMiddleware::class); + $dnsMiddleware = static fn (callable $handler): callable => $handler; $dnsPinMiddleware ->expects($this->atLeastOnce()) ->method('addDnsPinning') - ->willReturn(function (): void { - }); + ->willReturn($dnsMiddleware); $remoteHostValidator = $this->createMock(IRemoteHostValidator::class); + $remoteHostValidator->method('isValid')->willReturn(true); $eventLogger = $this->createMock(IEventLogger::class); + $eventLogger->expects($this->once()) + ->method('start') + ->with('http:request', 'GET request to /'); + $eventLogger->expects($this->once()) + ->method('end') + ->with('http:request'); $logger = $this->createMock(LoggerInterface::class); $serverVersion = $this->createMock(ServerVersion::class); + $serverVersion->method('getVersionString')->willReturn('1.0.0'); + $config->method('getSystemValueString')->willReturnMap([ + ['proxy', '', ''], + ['overwrite.cli.url', '', ''], + ]); + $config->method('getSystemValue')->with('proxyexclude', [])->willReturn([]); + $certificateManager->method('getDefaultCertificatesBundlePath')->willReturn('/tmp/certificates.crt'); $clientService = new ClientService( $config, @@ -58,27 +73,22 @@ public function testNewClient(): void { $serverVersion, ); - $handler = new CurlHandler(); - $stack = HandlerStack::create($handler); - $stack->push($dnsPinMiddleware->addDnsPinning()); - $stack->push(Middleware::tap(function (RequestInterface $request) use ($eventLogger): void { - $eventLogger->start('http:request', $request->getMethod() . ' request to ' . $request->getRequestTarget()); - }, function () use ($eventLogger): void { - $eventLogger->end('http:request'); - }), 'event logger'); - $guzzleClient = new GuzzleClient(['handler' => $stack]); + $client = $clientService->newClient(); + $this->assertEquals(new Client( + $config, + $certificateManager, + $this->getGuzzleClient($client), + $remoteHostValidator, + $logger, + $serverVersion, + ), $client); + + $stack = $this->getHandlerStack($client); + $this->assertStringContainsString("Name: ''", (string)$stack); + $this->assertStringContainsString("Name: 'event logger'", (string)$stack); - $this->assertEquals( - new Client( - $config, - $certificateManager, - $guzzleClient, - $remoteHostValidator, - $logger, - $serverVersion, - ), - $clientService->newClient() - ); + $stack->setHandler(new MockHandler([new Response(200)])); + $this->assertSame(200, $client->get('https://example.com')->getStatusCode()); } public function testDisableDnsPinning(): void { @@ -93,12 +103,25 @@ public function testDisableDnsPinning(): void { $dnsPinMiddleware ->expects($this->never()) ->method('addDnsPinning') - ->willReturn(function (): void { - }); + ->willReturn(static fn (callable $handler): callable => $handler); $remoteHostValidator = $this->createMock(IRemoteHostValidator::class); + $remoteHostValidator->method('isValid')->willReturn(true); $eventLogger = $this->createMock(IEventLogger::class); $logger = $this->createMock(LoggerInterface::class); $serverVersion = $this->createMock(ServerVersion::class); + $serverVersion->method('getVersionString')->willReturn('1.0.0'); + $config->method('getSystemValueBool')->willReturnMap([ + ['dns_pinning', true, false], + ['installed', false, false], + ['allow_local_remote_servers', false, false], + ['http_client_add_user_agent_url', false, false], + ]); + $config->method('getSystemValueString')->willReturnMap([ + ['proxy', '', ''], + ['overwrite.cli.url', '', ''], + ]); + $config->method('getSystemValue')->with('proxyexclude', [])->willReturn([]); + $certificateManager->method('getDefaultCertificatesBundlePath')->willReturn('/tmp/certificates.crt'); $clientService = new ClientService( $config, @@ -110,25 +133,28 @@ public function testDisableDnsPinning(): void { $serverVersion, ); - $handler = new CurlHandler(); - $stack = HandlerStack::create($handler); - $stack->push(Middleware::tap(function (RequestInterface $request) use ($eventLogger): void { - $eventLogger->start('http:request', $request->getMethod() . ' request to ' . $request->getRequestTarget()); - }, function () use ($eventLogger): void { - $eventLogger->end('http:request'); - }), 'event logger'); - $guzzleClient = new GuzzleClient(['handler' => $stack]); + $client = $clientService->newClient(); + $this->assertEquals(new Client( + $config, + $certificateManager, + $this->getGuzzleClient($client), + $remoteHostValidator, + $logger, + $serverVersion, + ), $client); - $this->assertEquals( - new Client( - $config, - $certificateManager, - $guzzleClient, - $remoteHostValidator, - $logger, - $serverVersion, - ), - $clientService->newClient() - ); + $stack = $this->getHandlerStack($client); + $this->assertStringNotContainsString("Name: ''", (string)$stack); + $this->assertStringContainsString("Name: 'event logger'", (string)$stack); + } + + private function getGuzzleClient(Client $client): \GuzzleHttp\Client { + return self::invokePrivate($client, 'client'); + } + + private function getHandlerStack(Client $client): HandlerStack { + /** @var HandlerStack $stack */ + $stack = $this->getGuzzleClient($client)->getConfig('handler'); + return $stack; } } diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index b2cecfdb0fff7..8aa8418e360e8 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -319,6 +319,23 @@ public function testGet(): void { $this->assertEquals(418, $this->client->get('http://localhost/', [])->getStatusCode()); } + public function testGetStreamUsesHttp11(): void { + $this->setUpDefaultRequestOptions(); + + $options = array_merge($this->defaultRequestOptions, [ + 'stream' => true, + 'version' => '1.1', + 'curl' => [ + \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_1_1, + ], + ]); + + $this->guzzleClient->method('request') + ->with('get', 'http://localhost/', $options) + ->willReturn(new Response(418)); + $this->assertEquals(418, $this->client->get('http://localhost/', ['stream' => true])->getStatusCode()); + } + public function testGetWithOptions(): void { $this->setUpDefaultRequestOptions(); @@ -520,6 +537,57 @@ public function testSetDefaultOptionsWithNotInstalled(): void { ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); } + public function testSetDefaultOptionsWithStream(): void { + $this->config + ->expects($this->exactly(3)) + ->method('getSystemValueBool') + ->willReturnMap([ + ['installed', false, true], + ['allow_local_remote_servers', false, false], + ['http_client_add_user_agent_url', false, false], + ]); + $this->config + ->expects($this->exactly(2)) + ->method('getSystemValueString') + ->willReturnMap([ + ['proxy', '', ''], + ['overwrite.cli.url', '', ''], + ]); + $this->certificateManager + ->expects($this->once()) + ->method('getAbsoluteBundlePath') + ->with() + ->willReturn('/my/path.crt'); + + $this->serverVersion->method('getVersionString') + ->willReturn('123.45.6'); + + $this->assertEquals([ + 'verify' => '/my/path.crt', + 'headers' => [ + 'User-Agent' => 'Nextcloud-Server-Crawler/123.45.6', + 'Accept-Encoding' => 'gzip', + ], + 'timeout' => 30, + 'nextcloud' => [ + 'allow_local_address' => false, + ], + 'allow_redirects' => [ + 'on_redirect' => function ( + \Psr\Http\Message\RequestInterface $request, + \Psr\Http\Message\ResponseInterface $response, + \Psr\Http\Message\UriInterface $uri, + ): void { + }, + ], + 'stream' => true, + 'version' => '1.1', + 'curl' => [ + \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_1_1, + ], + ], self::invokePrivate($this->client, 'buildRequestOptions', [['stream' => true]])); + } + public function testSetDefaultOptionsWithProxy(): void { $this->config ->expects($this->exactly(3))