From 6a16b91ae43b73b8b4e151d8ddc6eb41f747dcf3 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 7 Sep 2026 10:19:14 +0200 Subject: [PATCH 1/3] fix: don't mangle the hostname when the setup wizard builds the https url `ltrim($base, 'http://')` passes a character list, not a prefix, so it strips any leading h, t, p, : or / of whatever follows as well. `http://push.example.com` came out as `https://ush.example.com`, and that url is what gets saved as `base_endpoint` and handed to the clients. The same method then probed `$base`, the plain http url it already had, rather than the https one it had just built, and returned `$base` from both branches of the try/catch. So the https preference the method exists for never applied on a first call, and on a second call it returned the cached (mangled) https url instead, making `getBaseUrl()` answer differently depending on how many times it had been called within the same command. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/SetupWizard.php | 27 ++++++++++++++++++--------- tests/lib/SetupWizardTest.php | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 tests/lib/SetupWizardTest.php diff --git a/lib/SetupWizard.php b/lib/SetupWizard.php index 0f4c4f18..ff6b3809 100644 --- a/lib/SetupWizard.php +++ b/lib/SetupWizard.php @@ -146,23 +146,32 @@ public function isBinaryRunningAtDefaultPort(): bool { return $this->isBinaryRunningAt('http://localhost:7867'); } + public static function toHttps(string $url): string { + if (str_starts_with($url, 'https://')) { + return $url; + } + if (str_starts_with($url, 'http://')) { + return 'https://' . substr($url, strlen('http://')); + } + return 'https://' . $url; + } + private function getBaseUrl(): string { $base = $this->config->getSystemValueString('overwrite.cli.url', ''); - if (strpos($base, 'https://') !== 0) { - $httpsBase = 'https://' . ltrim($base, 'http://'); - if (isset($this->httpsCache[$httpsBase])) { - return ($this->httpsCache[$httpsBase]) ? $httpsBase : $base; - } + if ($base === '' || str_starts_with($base, 'https://')) { + return $base; + } + + $httpsBase = self::toHttps($base); + if (!isset($this->httpsCache[$httpsBase])) { try { - $this->client->get($base, ['nextcloud' => ['allow_local_address' => true], 'verify' => false]); + $this->client->get($httpsBase, ['nextcloud' => ['allow_local_address' => true], 'verify' => false]); $this->httpsCache[$httpsBase] = true; - return $base; } catch (\Exception $e) { $this->httpsCache[$httpsBase] = false; - return $base; } } - return $base; + return $this->httpsCache[$httpsBase] ? $httpsBase : $base; } public function isSelfSigned(): bool { diff --git a/tests/lib/SetupWizardTest.php b/tests/lib/SetupWizardTest.php new file mode 100644 index 00000000..2588b159 --- /dev/null +++ b/tests/lib/SetupWizardTest.php @@ -0,0 +1,32 @@ +assertEquals($expected, SetupWizard::toHttps($input)); + } +} From b5f4904afc743ae3fc0e613f1bc13013fd255cf2 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 7 Sep 2026 11:46:46 +0200 Subject: [PATCH 2/3] test: cover getBaseUrl itself, and handle an uppercase or missing scheme `toHttps` was tested but `getBaseUrl` was not, which left the actual bug this branch fixes, probing the wrong url and answering differently on a second call, with no test pinning it. Drive it through `getProxiedBase()` with a mocked client that records what it was asked for. Four of the six assertions fail against master, including one that catches both halves at once: the first call returns `http://push.example.com/push` and the second `https://ush.example.com/push`. `str_starts_with` is case sensitive, so `HTTP://host` fell through to the default arm and came out as `https://HTTP://host`, and a protocol relative `//host` became `https:////host`. Compare the scheme case insensitively and give `//host` its own arm. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/SetupWizard.php | 9 ++- tests/lib/SetupWizardBaseUrlTest.php | 96 ++++++++++++++++++++++++++++ tests/lib/SetupWizardTest.php | 5 ++ 3 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 tests/lib/SetupWizardBaseUrlTest.php diff --git a/lib/SetupWizard.php b/lib/SetupWizard.php index ff6b3809..8b868599 100644 --- a/lib/SetupWizard.php +++ b/lib/SetupWizard.php @@ -147,12 +147,17 @@ public function isBinaryRunningAtDefaultPort(): bool { } public static function toHttps(string $url): string { - if (str_starts_with($url, 'https://')) { + // the scheme is case insensitive, the rest of the url is not + $scheme = strtolower($url); + if (str_starts_with($scheme, 'https://')) { return $url; } - if (str_starts_with($url, 'http://')) { + if (str_starts_with($scheme, 'http://')) { return 'https://' . substr($url, strlen('http://')); } + if (str_starts_with($url, '//')) { + return 'https:' . $url; + } return 'https://' . $url; } diff --git a/tests/lib/SetupWizardBaseUrlTest.php b/tests/lib/SetupWizardBaseUrlTest.php new file mode 100644 index 00000000..194f95c5 --- /dev/null +++ b/tests/lib/SetupWizardBaseUrlTest.php @@ -0,0 +1,96 @@ +requested = []; + + $config = $this->createMock(IConfig::class); + $config->method('getSystemValueString') + ->willReturnCallback(function (string $key, string $default = '') use ($cliUrl) { + return $key === 'overwrite.cli.url' ? $cliUrl : $default; + }); + + $client = $this->createMock(IClient::class); + $client->method('get')->willReturnCallback(function (string $url) use ($httpsReachable) { + $this->requested[] = $url; + if (!$httpsReachable) { + throw new \Exception('connection refused'); + } + return $this->createMock(\OCP\Http\Client\IResponse::class); + }); + + $clientService = $this->createMock(IClientService::class); + $clientService->method('newClient')->willReturn($client); + + return new SetupWizard( + $this->createMock(IQueue::class), + $this->createMock(SelfTest::class), + $clientService, + $config, + $this->createMock(BinaryFinder::class), + ); + } + + public function testProbesTheHttpsUrlNotTheHttpOne(): void { + $wizard = $this->wizard('http://push.example.com', true); + $wizard->getProxiedBase(); + + $this->assertEquals( + ['https://push.example.com'], + $this->requested, + 'the wizard has to probe the https url it built, not the http one it started from' + ); + } + + public function testUsesHttpsWhenReachable(): void { + $wizard = $this->wizard('http://push.example.com', true); + $this->assertEquals('https://push.example.com/push', $wizard->getProxiedBase()); + } + + public function testFallsBackToHttpWhenHttpsIsNotReachable(): void { + $wizard = $this->wizard('http://push.example.com', false); + $this->assertEquals('http://push.example.com/push', $wizard->getProxiedBase()); + } + + public function testAnswersTheSameOnEveryCall(): void { + $wizard = $this->wizard('http://push.example.com', true); + + $first = $wizard->getProxiedBase(); + $second = $wizard->getProxiedBase(); + + $this->assertEquals($first, $second, 'the base url changed between calls'); + $this->assertCount(1, $this->requested, 'the https probe was not cached'); + } + + public function testHttpsUrlIsLeftAlone(): void { + $wizard = $this->wizard('https://push.example.com', true); + $this->assertEquals('https://push.example.com/push', $wizard->getProxiedBase()); + $this->assertCount(0, $this->requested, 'no probe is needed for a url that is already https'); + } + + public function testEmptyUrlIsNotProbed(): void { + $wizard = $this->wizard('', true); + $this->assertEquals('/push', $wizard->getProxiedBase()); + $this->assertCount(0, $this->requested); + } +} diff --git a/tests/lib/SetupWizardTest.php b/tests/lib/SetupWizardTest.php index 2588b159..4fcb6734 100644 --- a/tests/lib/SetupWizardTest.php +++ b/tests/lib/SetupWizardTest.php @@ -20,6 +20,11 @@ public function httpsUrlProvider(): array { ['http://tholos.example.com/nextcloud', 'https://tholos.example.com/nextcloud'], ['https://cloud.example.com', 'https://cloud.example.com'], ['cloud.example.com', 'https://cloud.example.com'], + // the scheme is case insensitive + ['HTTP://push.example.com', 'https://push.example.com'], + ['HTTPS://push.example.com', 'HTTPS://push.example.com'], + // protocol relative already carries its own separator + ['//push.example.com', 'https://push.example.com'], ]; } From 8d25b432e697f3355fecf8b57812356074f26c21 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:41:45 +0200 Subject: [PATCH 3/3] refactor: one anchored replace instead of four scheme branches `toHttps` was four `str_starts_with` arms doing what an anchored optional scheme group does in one line, and it now normalises an uppercase scheme rather than passing it through, which is what the caller wanted anyway. That also lets `getBaseUrl` drop its own `str_starts_with($base, 'https://')`: comparing the converted url against the original says the same thing and stays correct for a scheme in any case. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/SetupWizard.php | 18 ++++-------------- tests/lib/SetupWizardTest.php | 2 +- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/lib/SetupWizard.php b/lib/SetupWizard.php index 8b868599..7110430d 100644 --- a/lib/SetupWizard.php +++ b/lib/SetupWizard.php @@ -147,27 +147,17 @@ public function isBinaryRunningAtDefaultPort(): bool { } public static function toHttps(string $url): string { - // the scheme is case insensitive, the rest of the url is not - $scheme = strtolower($url); - if (str_starts_with($scheme, 'https://')) { - return $url; - } - if (str_starts_with($scheme, 'http://')) { - return 'https://' . substr($url, strlen('http://')); - } - if (str_starts_with($url, '//')) { - return 'https:' . $url; - } - return 'https://' . $url; + // replaces the scheme if there is one, adds it if there is not + return (string)preg_replace('#^(https?://|//)?#i', 'https://', $url, 1); } private function getBaseUrl(): string { $base = $this->config->getSystemValueString('overwrite.cli.url', ''); - if ($base === '' || str_starts_with($base, 'https://')) { + $httpsBase = self::toHttps($base); + if ($base === '' || $httpsBase === $base) { return $base; } - $httpsBase = self::toHttps($base); if (!isset($this->httpsCache[$httpsBase])) { try { $this->client->get($httpsBase, ['nextcloud' => ['allow_local_address' => true], 'verify' => false]); diff --git a/tests/lib/SetupWizardTest.php b/tests/lib/SetupWizardTest.php index 4fcb6734..ad420ae9 100644 --- a/tests/lib/SetupWizardTest.php +++ b/tests/lib/SetupWizardTest.php @@ -22,7 +22,7 @@ public function httpsUrlProvider(): array { ['cloud.example.com', 'https://cloud.example.com'], // the scheme is case insensitive ['HTTP://push.example.com', 'https://push.example.com'], - ['HTTPS://push.example.com', 'HTTPS://push.example.com'], + ['HTTPS://push.example.com', 'https://push.example.com'], // protocol relative already carries its own separator ['//push.example.com', 'https://push.example.com'], ];