From e004fb7042601384012aa326190eaf506e67a785 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Tue, 2 Jul 2024 20:34:07 +0200 Subject: [PATCH 1/2] fix: add option to remove the webroot for setup checks and don't check trusted_domains. 1) The checks for well-known urls should always run against the root domain and therefore the option to remove the webroot. 2) For trusted domains, the available protocol is unknown, and thus some guesswork would be needed to make that work. I've decided for now to not consider them anymore to reduce false-positives. Signed-off-by: Daniel Kesselberg --- .../SetupChecks/CheckServerResponseTrait.php | 49 ++++++++++++++----- .../settings/lib/SetupChecks/OcxProviders.php | 2 +- .../lib/SetupChecks/WellKnownUrls.php | 2 +- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php index e78ec602209..aed81a41ce5 100644 --- a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php +++ b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php @@ -52,26 +52,51 @@ trait CheckServerResponseTrait { /** * Get all possible URLs that need to be checked for a local request test. - * This takes all `trusted_domains` and the CLI overwrite URL into account. * * @param string $url The relative URL to test * @return string[] List of possible absolute URLs */ - protected function getTestUrls(string $url): array { - $hosts = $this->config->getSystemValue('trusted_domains', []); - $cliUrl = $this->config->getSystemValue('overwrite.cli.url', ''); - if ($cliUrl !== '') { - $hosts[] = $cliUrl; - } + protected function getTestUrls(string $url, bool $removeWebroot): array { + $testUrls = []; - $testUrls = array_merge( - [$this->urlGenerator->getAbsoluteURL($url)], - array_map(fn (string $host): string => $host . $url, $hosts), + $webroot = $this->urlGenerator->getWebroot(); + + $baseUrl = $this->normalizeUrl( + $this->urlGenerator->getBaseUrl(), + $webroot, + $removeWebroot ); + $testUrls[] = $baseUrl . $url; + + $cliUrl = $this->config->getSystemValueString('overwrite.cli.url', ''); + if ($cliUrl === '') { + return $testUrls; + } + + $cliUrl = $this->normalizeUrl( + $cliUrl, + $webroot, + $removeWebroot + ); + + if ($cliUrl !== $baseUrl) { + $testUrls[] = $cliUrl . $url; + } + return $testUrls; } + /** + * Strip a trailing slash and remove the webroot if requested. + */ + protected function normalizeUrl(string $url, string $webroot, bool $removeWebroot): string { + if ($removeWebroot && str_contains($url, $webroot)) { + $url = substr($url, -strlen($webroot)); + } + return rtrim($url, '/'); + } + /** * Run a HTTP request to check header * @param string $method The HTTP method to use @@ -86,14 +111,14 @@ trait CheckServerResponseTrait { * * @return Generator */ - protected function runRequest(string $method, string $url, array $options = []): Generator { + protected function runRequest(string $method, string $url, array $options = [], bool $removeWebroot = false): Generator { $options = array_merge(['ignoreSSL' => true, 'httpErrors' => true], $options); $client = $this->clientService->newClient(); $requestOptions = $this->getRequestOptions($options['ignoreSSL'], $options['httpErrors']); $requestOptions = array_merge($requestOptions, $options['options'] ?? []); - foreach ($this->getTestUrls($url) as $testURL) { + foreach ($this->getTestUrls($url, $removeWebroot) as $testURL) { try { yield $client->request($method, $testURL, $requestOptions); } catch (\Throwable $e) { diff --git a/apps/settings/lib/SetupChecks/OcxProviders.php b/apps/settings/lib/SetupChecks/OcxProviders.php index f387fe23a32..3a2863d441d 100644 --- a/apps/settings/lib/SetupChecks/OcxProviders.php +++ b/apps/settings/lib/SetupChecks/OcxProviders.php @@ -68,7 +68,7 @@ class OcxProviders implements ISetupCheck { ]; foreach ($providers as $provider) { - foreach ($this->runRequest('HEAD', $this->urlGenerator->getWebroot() . $provider, ['httpErrors' => false]) as $response) { + foreach ($this->runRequest('HEAD', $provider, ['httpErrors' => false]) as $response) { $testedProviders[$provider] = true; if ($response->getStatusCode() === 200) { $workingProviders[] = $provider; diff --git a/apps/settings/lib/SetupChecks/WellKnownUrls.php b/apps/settings/lib/SetupChecks/WellKnownUrls.php index 9471867c866..24f3c2d84e2 100644 --- a/apps/settings/lib/SetupChecks/WellKnownUrls.php +++ b/apps/settings/lib/SetupChecks/WellKnownUrls.php @@ -70,7 +70,7 @@ class WellKnownUrls implements ISetupCheck { foreach ($urls as [$verb,$url,$validStatuses,$checkCustomHeader]) { $works = null; - foreach ($this->runRequest($verb, $url, ['httpErrors' => false, 'options' => ['allow_redirects' => ['track_redirects' => true]]]) as $response) { + foreach ($this->runRequest($verb, $url, ['httpErrors' => false, 'options' => ['allow_redirects' => ['track_redirects' => true]]], removeWebroot: true) as $response) { // Check that the response status matches $works = in_array($response->getStatusCode(), $validStatuses); // and (if needed) the custom Nextcloud header is set From 48c807e667d28dfde56bce815be5489872d616d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 5 Sep 2024 11:54:45 +0200 Subject: [PATCH 2/2] fix(setupchecks): Test overwrite.cli url first, then generated one, and MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit trusted domains as last fallback. Signed-off-by: Côme Chilliet --- .../SetupChecks/CheckServerResponseTrait.php | 46 ++++++++++++------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php index aed81a41ce5..a62fe236369 100644 --- a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php +++ b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php @@ -52,36 +52,49 @@ trait CheckServerResponseTrait { /** * Get all possible URLs that need to be checked for a local request test. + * This takes all `trusted_domains` and the CLI overwrite URL into account. * - * @param string $url The relative URL to test + * @param string $url The relative URL to test starting with a / * @return string[] List of possible absolute URLs */ protected function getTestUrls(string $url, bool $removeWebroot): array { $testUrls = []; - $webroot = $this->urlGenerator->getWebroot(); + $webroot = rtrim($this->urlGenerator->getWebroot(), '/'); + /* Try overwrite.cli.url first, it’s supposed to be how the server contacts itself */ + $cliUrl = $this->config->getSystemValueString('overwrite.cli.url', ''); + + if ($cliUrl !== '') { + $cliUrl = $this->normalizeUrl( + $cliUrl, + $webroot, + $removeWebroot + ); + + $testUrls[] = $cliUrl . $url; + } + + /* Try URL generator second */ $baseUrl = $this->normalizeUrl( $this->urlGenerator->getBaseUrl(), $webroot, $removeWebroot ); - $testUrls[] = $baseUrl . $url; - - $cliUrl = $this->config->getSystemValueString('overwrite.cli.url', ''); - if ($cliUrl === '') { - return $testUrls; + if ($baseUrl !== $cliUrl) { + $testUrls[] = $baseUrl . $url; } - $cliUrl = $this->normalizeUrl( - $cliUrl, - $webroot, - $removeWebroot - ); - - if ($cliUrl !== $baseUrl) { - $testUrls[] = $cliUrl . $url; + /* Last resort: trusted domains */ + $hosts = $this->config->getSystemValue('trusted_domains', []); + foreach ($hosts as $host) { + if (str_contains($host, '*')) { + /* Ignore domains with a wildcard */ + continue; + } + $hosts[] = 'https://' . $host . $url; + $hosts[] = 'http://' . $host . $url; } return $testUrls; @@ -91,7 +104,8 @@ trait CheckServerResponseTrait { * Strip a trailing slash and remove the webroot if requested. */ protected function normalizeUrl(string $url, string $webroot, bool $removeWebroot): string { - if ($removeWebroot && str_contains($url, $webroot)) { + $url = rtrim($url, '/'); + if ($removeWebroot && str_ends_with($url, $webroot)) { $url = substr($url, -strlen($webroot)); } return rtrim($url, '/');