From e10e37bd727d97d58c4a67f12b3c522340d990ab Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 15 Apr 2026 11:29:11 -0400 Subject: [PATCH 1/7] refactor(http-client): extract request option helpers in Client Signed-off-by: Josh --- lib/private/Http/Client/Client.php | 112 +++++++++++++++++++---------- 1 file changed, 73 insertions(+), 39 deletions(-) diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index f1b81df396477..8e089cb42cd59 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -21,6 +21,7 @@ use OCP\ServerVersion; use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\UriInterface; use Psr\Log\LoggerInterface; use function parse_url; @@ -41,67 +42,100 @@ public function __construct( } private function buildRequestOptions(array $options): array { - $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', + 'curl' => [ + \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_2TLS, + ], ]; - $defaults['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_2TLS; - - $options['nextcloud']['allow_local_address'] = $this->isLocalAddressAllowed($options); - if ($options['nextcloud']['allow_local_address'] === false) { - $onRedirectFunction = function ( - \Psr\Http\Message\RequestInterface $request, - \Psr\Http\Message\ResponseInterface $response, - \Psr\Http\Message\UriInterface $uri, - ) use ($options): void { - $this->preventLocalAddress($uri->__toString(), $options); - }; - $defaults[RequestOptions::ALLOW_REDIRECTS] = [ - 'on_redirect' => $onRedirectFunction - ]; + $this->applyLocalAddressProtection($defaults, $options); + $this->applyProxyOption($defaults); + + $options = array_merge($defaults, $options); + $options[RequestOptions::HEADERS] ??= []; + + $this->ensureDefaultUserAgent($options); + $this->ensureDefaultAcceptEncoding($options); + $this->normalizeLegacySaveToOption($options); + + return $options; + } + + /** + * Propagate local-address policy into request options and validate redirect targets. + */ + private function applyLocalAddressProtection(array &$defaults, array &$options): void { + $allowLocalAddress = $this->isLocalAddressAllowed($options); + $options['nextcloud']['allow_local_address'] = $allowLocalAddress; + + if ($allowLocalAddress) { + return; } + $defaults[RequestOptions::ALLOW_REDIRECTS] = [ + 'on_redirect' => function ( + RequestInterface $_request, + ResponseInterface $_response, + UriInterface $uri, + ) use ($options): void { + $this->preventLocalAddress((string)$uri, $options); + }, + ]; + } + + private function applyProxyOption(array &$defaults): void { + $proxy = $this->getProxyUri(); + // Only add RequestOptions::PROXY if Nextcloud is explicitly // configured to use a proxy. This is needed in order not to override // Guzzle default values. if ($proxy !== null) { $defaults[RequestOptions::PROXY] = $proxy; } + } - $options = array_merge($defaults, $options); + private function ensureDefaultUserAgent(array &$options): void { + if (isset($options[RequestOptions::HEADERS]['User-Agent'])) { + return; + } - if (!isset($options[RequestOptions::HEADERS]['User-Agent'])) { - $userAgent = 'Nextcloud-Server-Crawler/' . $this->serverVersion->getVersionString(); - $overwriteCliUrl = $this->config->getSystemValueString('overwrite.cli.url'); - if ($this->config->getSystemValueBool('http_client_add_user_agent_url') && !empty($overwriteCliUrl)) { - $userAgent .= '; +' . rtrim($overwriteCliUrl, '/'); - } - $options[RequestOptions::HEADERS]['User-Agent'] = $userAgent; + $userAgent = 'Nextcloud-Server-Crawler/' . $this->serverVersion->getVersionString(); + $overwriteCliUrl = $this->config->getSystemValueString('overwrite.cli.url'); + $addUrl = !empty($overwriteCliUrl) && $this->config->getSystemValueBool('http_client_add_user_agent_url', false); + + // Optionally append the instance URL to the crawler user agent. + if ($addUrl) { + $userAgent .= '; +' . rtrim($overwriteCliUrl, '/'); } - // Ensure headers array exists and set Accept-Encoding only if not present - $headers = $options[RequestOptions::HEADERS] ?? []; - if (!isset($headers['Accept-Encoding'])) { - $acceptEnc = 'gzip'; - if (function_exists('brotli_uncompress')) { - $acceptEnc = 'br, ' . $acceptEnc; - } - $options[RequestOptions::HEADERS] = $headers; // ensure headers are present - $options[RequestOptions::HEADERS]['Accept-Encoding'] = $acceptEnc; + $options[RequestOptions::HEADERS]['User-Agent'] = $userAgent; + } + + private function ensureDefaultAcceptEncoding(array &$options): void { + if (isset($options[RequestOptions::HEADERS]['Accept-Encoding'])) { + return; } - // Fallback for save_to - if (isset($options['save_to'])) { - $options['sink'] = $options['save_to']; - unset($options['save_to']); + $acceptEncoding = 'gzip'; + if (function_exists('brotli_uncompress')) { + $acceptEncoding = 'br, ' . $acceptEncoding; } - return $options; + $options[RequestOptions::HEADERS]['Accept-Encoding'] = $acceptEncoding; + } + + private function normalizeLegacySaveToOption(array &$options): void { + if (!isset($options['save_to'])) { + return; + } + + // Support legacy 'save_to' by mapping it to Guzzle's 'sink'. + $options['sink'] = $options['save_to']; + unset($options['save_to']); } private function getCertBundle(): string { @@ -155,7 +189,7 @@ private function getProxyUri(): ?array { return $proxy; } - private function isLocalAddressAllowed(array $options) : bool { + private function isLocalAddressAllowed(array $options): bool { if (($options['nextcloud']['allow_local_address'] ?? false) || $this->config->getSystemValueBool('allow_local_remote_servers', false)) { return true; From f0d0b0b0afb5d9f859ca5810da6148fe6ef97a75 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 15 Apr 2026 11:56:44 -0400 Subject: [PATCH 2/7] fix(http-client): only enable HTTP/2 when supported by libcurl Fixes #58528 Signed-off-by: Josh --- lib/private/Http/Client/Client.php | 35 +++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index 8e089cb42cd59..f82ee51a0a5c5 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -45,13 +45,9 @@ private function buildRequestOptions(array $options): array { $defaults = [ RequestOptions::VERIFY => $this->getCertBundle(), RequestOptions::TIMEOUT => IClient::DEFAULT_REQUEST_TIMEOUT, - // Prefer HTTP/2 globally (PSR-7 request version) - RequestOptions::VERSION => '2.0', - 'curl' => [ - \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_2TLS, - ], ]; + $this->applyHttp2Defaults($defaults); $this->applyLocalAddressProtection($defaults, $options); $this->applyProxyOption($defaults); @@ -138,6 +134,35 @@ private function normalizeLegacySaveToOption(array &$options): void { unset($options['save_to']); } + private function applyHttp2Defaults(array &$defaults): void { + if (!$this->supportsHttp2()) { + return; + } + + // Prefer HTTP/2 when supported by the local libcurl build. + $defaults[RequestOptions::VERSION] = '2.0'; + + if (\defined('CURL_HTTP_VERSION_2TLS')) { + $defaults['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_2TLS; + return; + } + + if (\defined('CURL_HTTP_VERSION_2_0')) { + $defaults['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_2_0; + } + } + + private function supportsHttp2(): bool { + if (!\function_exists('curl_version') || !\defined('CURL_VERSION_HTTP2')) { + return false; + } + + $curlVersion = \curl_version(); + $features = $curlVersion['features'] ?? 0; + + return ($features & \CURL_VERSION_HTTP2) === \CURL_VERSION_HTTP2; + } + private function getCertBundle(): string { // If the instance is not yet setup we need to use the static path as // $this->certificateManager->getAbsoluteBundlePath() tries to instantiate From ebd8becc00468ac68f27815ca4620a13657893ec Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 15 Apr 2026 17:29:53 -0400 Subject: [PATCH 3/7] test(http-client): change supportsHttp2 method visibility Signed-off-by: Josh --- lib/private/Http/Client/Client.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index f82ee51a0a5c5..44217de6f2099 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -152,7 +152,7 @@ private function applyHttp2Defaults(array &$defaults): void { } } - private function supportsHttp2(): bool { + protected function supportsHttp2(): bool { if (!\function_exists('curl_version') || !\defined('CURL_VERSION_HTTP2')) { return false; } From 6e5177ee6427c3e177c95a3d4a2026ff10ddb217 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 15 Apr 2026 18:16:31 -0400 Subject: [PATCH 4/7] test(http-client): Add explicit coverage for HTTP/2 scenarios Signed-off-by: Josh --- tests/lib/Http/Client/ClientTest.php | 165 +++++++++++++++++++++------ 1 file changed, 128 insertions(+), 37 deletions(-) diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index ed732cdddc3f6..d782f56c55b58 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -51,14 +51,37 @@ protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); $this->serverVersion = $this->createMock(ServerVersion::class); - $this->client = new Client( - $this->config, - $this->certificateManager, - $this->guzzleClient, - $this->remoteHostValidator, - $this->logger, - $this->serverVersion, - ); + $this->client = $this->newClientWithHttp2Support(true); + } + + private function newClientWithHttp2Support(bool $supportsHttp2): Client { + $client = $this->getMockBuilder(Client::class) + ->setConstructorArgs([ + $this->config, + $this->certificateManager, + $this->guzzleClient, + $this->remoteHostValidator, + $this->logger, + $this->serverVersion, + ]) + ->onlyMethods(['supportsHttp2']) + ->getMock(); + + $client->method('supportsHttp2') + ->willReturn($supportsHttp2); + + return $client; + } + + private function addExpectedHttp2Options(array $options): array { + $options['version'] = '2.0'; + $options['curl'] = [ + \CURLOPT_HTTP_VERSION => \defined('CURL_HTTP_VERSION_2TLS') + ? \CURL_HTTP_VERSION_2TLS + : \CURL_HTTP_VERSION_2_0, + ]; + + return $options; } public function testGetProxyUri(): void { @@ -257,7 +280,9 @@ public function testPreventLocalAddressOnDelete(string $uri): void { $this->client->delete($uri); } - private function setUpDefaultRequestOptions(): void { + private function setUpDefaultRequestOptions(bool $supportsHttp2 = true): void { + $this->client = $this->newClientWithHttp2Support($supportsHttp2); + $this->config ->method('getSystemValue') ->willReturnMap([ @@ -288,6 +313,7 @@ private function setUpDefaultRequestOptions(): void { ->willReturn('123.45.6'); $acceptEnc = function_exists('brotli_uncompress') ? 'br, gzip' : 'gzip'; + $this->defaultRequestOptions = [ 'verify' => '/my/path.crt', 'proxy' => [ @@ -298,17 +324,16 @@ private function setUpDefaultRequestOptions(): void { 'User-Agent' => 'Nextcloud-Server-Crawler/123.45.6', 'Accept-Encoding' => $acceptEnc, - ], 'timeout' => 30, 'nextcloud' => [ 'allow_local_address' => true, ], - 'version' => '2.0', - 'curl' => [ - \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_2TLS, - ], ]; + + if ($supportsHttp2) { + $this->defaultRequestOptions = $this->addExpectedHttp2Options($this->defaultRequestOptions); + } } public function testGet(): void { @@ -468,6 +493,8 @@ public function testHeadWithOptions(): void { } public function testSetDefaultOptionsWithNotInstalled(): void { + $this->client = $this->newClientWithHttp2Support(true); + $this->config ->expects($this->exactly(3)) ->method('getSystemValueBool') @@ -483,6 +510,7 @@ public function testSetDefaultOptionsWithNotInstalled(): void { ['proxy', '', ''], ['overwrite.cli.url', '', ''], ]); + $this->certificateManager ->expects($this->never()) ->method('listCertificates'); @@ -496,7 +524,7 @@ public function testSetDefaultOptionsWithNotInstalled(): void { $acceptEnc = function_exists('brotli_uncompress') ? 'br, gzip' : 'gzip'; - $this->assertEquals([ + $expected = [ 'verify' => \OC::$SERVERROOT . '/resources/config/ca-bundle.crt', 'headers' => [ 'User-Agent' => 'Nextcloud-Server-Crawler/123.45.6', @@ -514,14 +542,16 @@ public function testSetDefaultOptionsWithNotInstalled(): void { ): void { }, ], - 'version' => '2.0', - 'curl' => [ - \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_2TLS, - ], - ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); + ]; + + $expected = $this->addExpectedHttp2Options($expected); + + $this->assertEquals($expected, self::invokePrivate($this->client, 'buildRequestOptions', [[]])); } public function testSetDefaultOptionsWithProxy(): void { + $this->client = $this->newClientWithHttp2Support(true); + $this->config ->expects($this->exactly(3)) ->method('getSystemValueBool') @@ -543,6 +573,7 @@ public function testSetDefaultOptionsWithProxy(): void { ['proxyuserpwd', '', ''], ['overwrite.cli.url', '', ''], ]); + $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') @@ -554,7 +585,7 @@ public function testSetDefaultOptionsWithProxy(): void { $acceptEnc = function_exists('brotli_uncompress') ? 'br, gzip' : 'gzip'; - $this->assertEquals([ + $expected = [ 'verify' => '/my/path.crt', 'proxy' => [ 'http' => 'foo', @@ -576,14 +607,16 @@ public function testSetDefaultOptionsWithProxy(): void { ): void { }, ], - 'version' => '2.0', - 'curl' => [ - \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_2TLS, - ], - ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); + ]; + + $expected = $this->addExpectedHttp2Options($expected); + + $this->assertEquals($expected, self::invokePrivate($this->client, 'buildRequestOptions', [[]])); } public function testSetDefaultOptionsWithProxyAndExclude(): void { + $this->client = $this->newClientWithHttp2Support(true); + $this->config ->expects($this->exactly(3)) ->method('getSystemValueBool') @@ -605,6 +638,7 @@ public function testSetDefaultOptionsWithProxyAndExclude(): void { ['proxyuserpwd', '', ''], ['overwrite.cli.url', '', ''], ]); + $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') @@ -616,7 +650,7 @@ public function testSetDefaultOptionsWithProxyAndExclude(): void { $acceptEnc = function_exists('brotli_uncompress') ? 'br, gzip' : 'gzip'; - $this->assertEquals([ + $expected = [ 'verify' => '/my/path.crt', 'proxy' => [ 'http' => 'foo', @@ -639,11 +673,63 @@ public function testSetDefaultOptionsWithProxyAndExclude(): void { ): void { }, ], - 'version' => '2.0', - 'curl' => [ - \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_2TLS, + ]; + + $expected = $this->addExpectedHttp2Options($expected); + + $this->assertEquals($expected, self::invokePrivate($this->client, 'buildRequestOptions', [[]])); + } + + public function testSetDefaultOptionsWithoutHttp2Support(): void { + $this->client = $this->newClientWithHttp2Support(false); + + $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') + ->willReturn('/my/path.crt'); + + $this->serverVersion->method('getVersionString') + ->willReturn('123.45.6'); + + $acceptEnc = function_exists('brotli_uncompress') ? 'br, gzip' : 'gzip'; + + $expected = [ + 'verify' => '/my/path.crt', + 'headers' => [ + 'User-Agent' => 'Nextcloud-Server-Crawler/123.45.6', + 'Accept-Encoding' => $acceptEnc, + ], + '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 { + }, ], - ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); + ]; + + $this->assertEquals($expected, self::invokePrivate($this->client, 'buildRequestOptions', [[]])); } public static function dataForTestSetServerUrlInUserAgent(): array { @@ -655,6 +741,8 @@ public static function dataForTestSetServerUrlInUserAgent(): array { #[DataProvider('dataForTestSetServerUrlInUserAgent')] public function testSetServerUrlInUserAgent(string $url, string $userAgent): void { + $this->client = $this->newClientWithHttp2Support(true); + $this->config ->expects($this->exactly(3)) ->method('getSystemValueBool') @@ -670,6 +758,7 @@ public function testSetServerUrlInUserAgent(string $url, string $userAgent): voi ['proxy', '', ''], ['overwrite.cli.url', '', $url], ]); + $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') @@ -679,11 +768,13 @@ public function testSetServerUrlInUserAgent(string $url, string $userAgent): voi $this->serverVersion->method('getVersionString') ->willReturn('123.45.6'); + $acceptEnc = function_exists('brotli_uncompress') ? 'br, gzip' : 'gzip'; + $this->assertEquals([ 'verify' => '/my/path.crt', 'headers' => [ 'User-Agent' => $userAgent, - 'Accept-Encoding' => 'gzip', + 'Accept-Encoding' => $acceptEnc, ], 'timeout' => 30, 'nextcloud' => [ @@ -697,10 +788,10 @@ public function testSetServerUrlInUserAgent(string $url, string $userAgent): voi ): void { }, ], - 'version' => '2.0', - 'curl' => [ - \CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_2TLS, - ], - ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); + ]; + + $expected = $this->addExpectedHttp2Options($expected); + + $this->assertEquals($expected, self::invokePrivate($this->client, 'buildRequestOptions', [[]])); } } From 191c9c1920bb8faadb010db650bfb181c22002e7 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 15 Apr 2026 18:19:36 -0400 Subject: [PATCH 5/7] chore(http-client): fix mistake in updated ClientTest Signed-off-by: Josh --- tests/lib/Http/Client/ClientTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index d782f56c55b58..1ab1a4b44f241 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -770,7 +770,7 @@ public function testSetServerUrlInUserAgent(string $url, string $userAgent): voi $acceptEnc = function_exists('brotli_uncompress') ? 'br, gzip' : 'gzip'; - $this->assertEquals([ + $expected = [ 'verify' => '/my/path.crt', 'headers' => [ 'User-Agent' => $userAgent, From f856bcba8b9ef848f4514b2762a4bc2672d3c126 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 15 Apr 2026 18:20:41 -0400 Subject: [PATCH 6/7] chore: fixup ClientTest cs/lint Signed-off-by: Josh --- tests/lib/Http/Client/ClientTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index 1ab1a4b44f241..a35d9c97f6712 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -543,7 +543,7 @@ public function testSetDefaultOptionsWithNotInstalled(): void { }, ], ]; - + $expected = $this->addExpectedHttp2Options($expected); $this->assertEquals($expected, self::invokePrivate($this->client, 'buildRequestOptions', [[]])); From e14c710892a469b31eba34f6aa6fe03f9a1fdd3e Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 15 Apr 2026 18:34:30 -0400 Subject: [PATCH 7/7] chore(http-client): drop call count which is an implementation detail Signed-off-by: Josh --- tests/lib/Http/Client/ClientTest.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index a35d9c97f6712..a36a7f97d0459 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -496,7 +496,6 @@ public function testSetDefaultOptionsWithNotInstalled(): void { $this->client = $this->newClientWithHttp2Support(true); $this->config - ->expects($this->exactly(3)) ->method('getSystemValueBool') ->willReturnMap([ ['installed', false, false], @@ -553,7 +552,6 @@ public function testSetDefaultOptionsWithProxy(): void { $this->client = $this->newClientWithHttp2Support(true); $this->config - ->expects($this->exactly(3)) ->method('getSystemValueBool') ->willReturnMap([ ['installed', false, true], @@ -618,7 +616,6 @@ public function testSetDefaultOptionsWithProxyAndExclude(): void { $this->client = $this->newClientWithHttp2Support(true); $this->config - ->expects($this->exactly(3)) ->method('getSystemValueBool') ->willReturnMap([ ['installed', false, true], @@ -684,7 +681,6 @@ public function testSetDefaultOptionsWithoutHttp2Support(): void { $this->client = $this->newClientWithHttp2Support(false); $this->config - ->expects($this->exactly(3)) ->method('getSystemValueBool') ->willReturnMap([ ['installed', false, true], @@ -744,7 +740,6 @@ public function testSetServerUrlInUserAgent(string $url, string $userAgent): voi $this->client = $this->newClientWithHttp2Support(true); $this->config - ->expects($this->exactly(3)) ->method('getSystemValueBool') ->willReturnMap([ ['installed', false, true],