From 4ce4d7b9b31268f3b9242eaa73be6390890ef761 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 | 47 ++++++++++++++----- .../settings/lib/SetupChecks/OcxProviders.php | 2 +- .../lib/SetupChecks/WellKnownUrls.php | 2 +- 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php index a77e0d87f2768..49a9bcee9d44c 100644 --- a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php +++ b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php @@ -35,26 +35,51 @@ protected function serverConfigHelp(): string { /** * 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 = []; + + $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; } - $testUrls = array_merge( - [$this->urlGenerator->getAbsoluteURL($url)], - array_map(fn (string $host): string => $host . $url, $hosts), + $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 @@ -69,14 +94,14 @@ protected function getTestUrls(string $url): array { * * @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 ecb8ecd660928..84da99dbfb097 100644 --- a/apps/settings/lib/SetupChecks/OcxProviders.php +++ b/apps/settings/lib/SetupChecks/OcxProviders.php @@ -51,7 +51,7 @@ public function run(): SetupResult { ]; 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 2b5481d16ffcc..565544cfdd790 100644 --- a/apps/settings/lib/SetupChecks/WellKnownUrls.php +++ b/apps/settings/lib/SetupChecks/WellKnownUrls.php @@ -52,7 +52,7 @@ public function run(): SetupResult { 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 c224b8ced4e8e9ea2b69eca49ba258c45542e346 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 49a9bcee9d44c..090e1322e2f29 100644 --- a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php +++ b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php @@ -35,36 +35,49 @@ protected function serverConfigHelp(): string { /** * 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(), '/'); - $baseUrl = $this->normalizeUrl( - $this->urlGenerator->getBaseUrl(), - $webroot, - $removeWebroot - ); + /* Try overwrite.cli.url first, it’s supposed to be how the server contacts itself */ + $cliUrl = $this->config->getSystemValueString('overwrite.cli.url', ''); - $testUrls[] = $baseUrl . $url; + if ($cliUrl !== '') { + $cliUrl = $this->normalizeUrl( + $cliUrl, + $webroot, + $removeWebroot + ); - $cliUrl = $this->config->getSystemValueString('overwrite.cli.url', ''); - if ($cliUrl === '') { - return $testUrls; + $testUrls[] = $cliUrl . $url; } - $cliUrl = $this->normalizeUrl( - $cliUrl, + /* Try URL generator second */ + $baseUrl = $this->normalizeUrl( + $this->urlGenerator->getBaseUrl(), $webroot, $removeWebroot ); - if ($cliUrl !== $baseUrl) { - $testUrls[] = $cliUrl . $url; + if ($baseUrl !== $cliUrl) { + $testUrls[] = $baseUrl . $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; @@ -74,7 +87,8 @@ protected function getTestUrls(string $url, bool $removeWebroot): array { * 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, '/');