diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index 0a42ace17c0b4..ea66013190aa8 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -41,15 +41,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) { @@ -75,6 +79,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 12d633a962be9..592429cc43c75 100644 --- a/lib/private/Http/Client/ClientService.php +++ b/lib/private/Http/Client/ClientService.php @@ -9,8 +9,8 @@ namespace OC\Http\Client; use GuzzleHttp\Client as GuzzleClient; -use GuzzleHttp\Handler\CurlHandler; use GuzzleHttp\HandlerStack; +use GuzzleHttp\Utils; use GuzzleHttp\Middleware; use OCP\Diagnostics\IEventLogger; use OCP\Http\Client\IClient; @@ -41,7 +41,7 @@ public function __construct( #[\Override] public function newClient(): IClient { - $handler = new CurlHandler(); + $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..889b03053d0dd 100644 --- a/tests/lib/Http/Client/ClientServiceTest.php +++ b/tests/lib/Http/Client/ClientServiceTest.php @@ -10,10 +10,6 @@ namespace Test\Http\Client; -use GuzzleHttp\Client as GuzzleClient; -use GuzzleHttp\Handler\CurlHandler; -use GuzzleHttp\HandlerStack; -use GuzzleHttp\Middleware; use OC\Http\Client\Client; use OC\Http\Client\ClientService; use OC\Http\Client\DnsPinMiddleware; @@ -22,7 +18,6 @@ use OCP\IConfig; use OCP\Security\IRemoteHostValidator; use OCP\ServerVersion; -use Psr\Http\Message\RequestInterface; use Psr\Log\LoggerInterface; /** @@ -58,27 +53,7 @@ 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]); - - $this->assertEquals( - new Client( - $config, - $certificateManager, - $guzzleClient, - $remoteHostValidator, - $logger, - $serverVersion, - ), - $clientService->newClient() - ); + $this->assertInstanceOf(Client::class, $clientService->newClient()); } public function testDisableDnsPinning(): void { @@ -110,25 +85,6 @@ 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]); - - $this->assertEquals( - new Client( - $config, - $certificateManager, - $guzzleClient, - $remoteHostValidator, - $logger, - $serverVersion, - ), - $clientService->newClient() - ); + $this->assertInstanceOf(Client::class, $clientService->newClient()); } } diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index c6fa45eb318f8..e68c86cfca217 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -321,6 +321,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(); @@ -522,6 +539,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))