fix(setup-checks): Ensure URL with webroot works

We basically mock the way `URLGenerator::getAbsoluteURL` works, so we must make sure that the URL might already contain the webroot. Because `baseURL` and `cliURL` also contain the webroot we need to remove the webroot from the URL first. Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de> Co-authored-by: Daniel <mail@danielkesselberg.de> Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
nextcloud · Oct 11, 2024 · 39d66ed · 39d66ed
1 parent 7e2b9a0
commit 39d66ed
Show file tree

Hide file tree

Showing 7 changed files with 284 additions and 28 deletions.
diff --git a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php
@@ -55,66 +55,67 @@ protected function serverConfigHelp(): string {
 	 * This takes all `trusted_domains` and the CLI overwrite URL into account.
 	 *
 	 * @param string $url The relative URL to test starting with a /
-	 * @return string[] List of possible absolute URLs
+	 * @return list<string> List of possible absolute URLs
 	 */
 	protected function getTestUrls(string $url, bool $removeWebroot): array {
-		$testUrls = [];
+		$url = '/' . ltrim($url, '/');
 
 		$webroot = rtrim($this->urlGenerator->getWebroot(), '/');
+		// Similar to `getAbsoluteURL` of URLGenerator:
+		// The Nextcloud web root could already be prepended.
+		if ($webroot !== '' && str_starts_with($url, $webroot)) {
+			$url = substr($url, strlen($webroot));
+		}
+
+		$hosts = [];
 
 		/* 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(
+			$hosts[] = $this->normalizeUrl(
 				$cliUrl,
 				$webroot,
 				$removeWebroot
 			);
-
-			$testUrls[] = $cliUrl . $url;
 		}
 
 		/* Try URL generator second */
-		$baseUrl = $this->normalizeUrl(
+		$hosts[] = $this->normalizeUrl(
 			$this->urlGenerator->getBaseUrl(),
 			$webroot,
 			$removeWebroot
 		);
 
-		if ($baseUrl !== $cliUrl) {
-			$testUrls[] = $baseUrl . $url;
-		}
-
 		/* Last resort: trusted domains */
-		$hosts = $this->config->getSystemValue('trusted_domains', []);
-		foreach ($hosts as $host) {
+		$trustedDomains = $this->config->getSystemValue('trusted_domains', []);
+		foreach ($trustedDomains as $host) {
 			if (str_contains($host, '*')) {
 				/* Ignore domains with a wildcard */
 				continue;
 			}
-			$hosts[] = 'https://' . $host . $url;
-			$hosts[] = 'http://' . $host . $url;
+			$hosts[] = $this->normalizeUrl("https://$host$webroot", $webroot, $removeWebroot);
+			$hosts[] = $this->normalizeUrl("http://$host$webroot", $webroot, $removeWebroot);
 		}
 
-		return $testUrls;
+		return array_map(fn (string $host) => $host . $url, array_values(array_unique($hosts)));
 	}
 
 	/**
 	 * Strip a trailing slash and remove the webroot if requested.
 	 */
 	protected function normalizeUrl(string $url, string $webroot, bool $removeWebroot): string {
 		$url = rtrim($url, '/');
-		if ($removeWebroot && str_ends_with($url, $webroot)) {
-			$url = substr($url, -strlen($webroot));
+		if ($removeWebroot && $webroot !== '' && str_ends_with($url, $webroot)) {
+			$url = substr($url, 0, -strlen($webroot));
 		}
 		return rtrim($url, '/');
 	}
 
 	/**
 	 * Run a HTTP request to check header
 	 * @param string $method The HTTP method to use
-	 * @param string $url The relative URL to check
+	 * @param string $url The relative URL to check (e.g. output of IURLGenerator)
+	 * @param bool $removeWebroot Remove the webroot from the URL (handle URL as relative to domain root)
 	 * @param array{ignoreSSL?: bool, httpErrors?: bool, options?: array} $options Additional options, like
 	 *                                                 [
 	 *                                                  // Ignore invalid SSL certificates (e.g. self signed)
@@ -143,13 +144,14 @@ protected function runRequest(string $method, string $url, array $options = [],
 
 	/**
 	 * Run a HEAD request to check header
-	 * @param string $url The relative URL to check
+	 * @param string $url The relative URL to check (e.g. output of IURLGenerator)
 	 * @param bool $ignoreSSL Ignore SSL certificates
 	 * @param bool $httpErrors Ignore requests with HTTP errors (will not yield if request has a 4xx or 5xx response)
+	 * @param bool $removeWebroot Remove the webroot from the URL (handle URL as relative to domain root)
 	 * @return Generator<int, IResponse>
 	 */
-	protected function runHEAD(string $url, bool $ignoreSSL = true, bool $httpErrors = true): Generator {
-		return $this->runRequest('HEAD', $url, ['ignoreSSL' => $ignoreSSL, 'httpErrors' => $httpErrors]);
+	protected function runHEAD(string $url, bool $ignoreSSL = true, bool $httpErrors = true, bool $removeWebroot = false): Generator {
+		return $this->runRequest('HEAD', $url, ['ignoreSSL' => $ignoreSSL, 'httpErrors' => $httpErrors], $removeWebroot);
 	}
 
 	protected function getRequestOptions(bool $ignoreSSL, bool $httpErrors): array {

diff --git a/apps/settings/lib/SetupChecks/DataDirectoryProtected.php b/apps/settings/lib/SetupChecks/DataDirectoryProtected.php
@@ -58,8 +58,7 @@ public function getName(): string {
 
 	public function run(): SetupResult {
 		$datadir = str_replace(\OC::$SERVERROOT . '/', '', $this->config->getSystemValue('datadirectory', ''));
-
-		$dataUrl = $this->urlGenerator->getWebroot() . '/' . $datadir . '/.ocdata';
+		$dataUrl = '/' . $datadir . '/.ocdata';
 
 		$noResponse = true;
 		foreach ($this->runHEAD($dataUrl, httpErrors:false) as $response) {

diff --git a/apps/settings/lib/SetupChecks/WellKnownUrls.php b/apps/settings/lib/SetupChecks/WellKnownUrls.php
@@ -68,9 +68,10 @@ public function run(): SetupResult {
 			['propfind', '/.well-known/carddav', [207], false],
 		];
 
+		$requestOptions = ['httpErrors' => false, 'options' => ['allow_redirects' => ['track_redirects' => true]]];
 		foreach ($urls as [$verb,$url,$validStatuses,$checkCustomHeader]) {
 			$works = null;
-			foreach ($this->runRequest($verb, $url, ['httpErrors' => false, 'options' => ['allow_redirects' => ['track_redirects' => true]]], removeWebroot: true) as $response) {
+			foreach ($this->runRequest($verb, $url, $requestOptions, 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

diff --git a/apps/settings/tests/SetupChecks/CheckServerResponseTraitImplementation.php b/apps/settings/tests/SetupChecks/CheckServerResponseTraitImplementation.php
@@ -0,0 +1,40 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\Settings\Tests\SetupChecks;
+
+use OCA\Settings\SetupChecks\CheckServerResponseTrait;
+use OCP\Http\Client\IClientService;
+use OCP\IConfig;
+use OCP\IL10N;
+use OCP\IURLGenerator;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Dummy implementation for CheckServerResponseTraitTest
+ */
+class CheckServerResponseTraitImplementation {
+
+	use CheckServerResponseTrait {
+		CheckServerResponseTrait::getRequestOptions as public;
+		CheckServerResponseTrait::runHEAD as public;
+		CheckServerResponseTrait::runRequest as public;
+		CheckServerResponseTrait::normalizeUrl as public;
+		CheckServerResponseTrait::getTestUrls as public;
+	}
+
+	public function __construct(
+		protected IL10N $l10n,
+		protected IConfig $config,
+		protected IURLGenerator $urlGenerator,
+		protected IClientService $clientService,
+		protected LoggerInterface $logger,
+	) {
+	}
+
+}
diff --git a/apps/settings/tests/SetupChecks/CheckServerResponseTraitTest.php b/apps/settings/tests/SetupChecks/CheckServerResponseTraitTest.php
@@ -0,0 +1,214 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\Settings\Tests\SetupChecks;
+
+use OCP\Http\Client\IClientService;
+use OCP\IConfig;
+use OCP\IL10N;
+use OCP\IURLGenerator;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
+use Test\TestCase;
+
+class CheckServerResponseTraitTest extends TestCase {
+
+	protected const BASE_URL = 'https://nextcloud.local';
+
+	private IL10N&MockObject $l10n;
+	private IConfig&MockObject $config;
+	private IURLGenerator&MockObject $urlGenerator;
+	private IClientService&MockObject $clientService;
+	private LoggerInterface&MockObject $logger;
+
+	private CheckServerResponseTraitImplementation $trait;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->l10n = $this->createMock(IL10N::class);
+		$this->l10n->method('t')
+			->willReturnArgument(0);
+		$this->config = $this->createMock(IConfig::class);
+		$this->urlGenerator = $this->createMock(IURLGenerator::class);
+		$this->clientService = $this->createMock(IClientService::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
+
+		$this->trait = new CheckServerResponseTraitImplementation(
+			$this->l10n,
+			$this->config,
+			$this->urlGenerator,
+			$this->clientService,
+			$this->logger,
+		);
+	}
+
+	/**
+	 * @dataProvider dataNormalizeUrl
+	 */
+	public function testNormalizeUrl(string $url, string $webRoot, bool $removeWebRoot, string $expected): void {
+		$this->assertEquals($expected, $this->trait->normalizeUrl($url, $webRoot, $removeWebRoot));
+	}
+
+	public static function dataNormalizeUrl(): array {
+		return [
+			'valid and nothing to change' => ['http://example.com/root', '/root', false, 'http://example.com/root'],
+			'trailing slash' => ['http://example.com/root/', '/root', false, 'http://example.com/root'],
+			'remove web root' => ['http://example.com/root/', '/root', true, 'http://example.com'],
+			'remove web root but empty' => ['http://example.com', '', true, 'http://example.com'],
+		];
+	}
+
+	/**
+	 * @dataProvider dataGetTestUrls
+	 */
+	public function testGetTestUrls(
+		string $url,
+		bool $removeWebRoot,
+		string $cliUrl,
+		string $webRoot,
+		array $trustedDomains,
+		array $expected,
+	): void {
+		$this->config->expects(self::atLeastOnce())
+			->method('getSystemValueString')
+			->with('overwrite.cli.url', '')
+			->willReturn($cliUrl);
+
+		$this->config->expects(self::atLeastOnce())
+			->method('getSystemValue')
+			->with('trusted_domains', [])
+			->willReturn($trustedDomains);
+
+		$this->urlGenerator->expects(self::atLeastOnce())
+			->method('getWebroot')
+			->willReturn($webRoot);
+
+		$this->urlGenerator->expects(self::atLeastOnce())
+			->method('getBaseUrl')
+			->willReturn(self::BASE_URL . $webRoot);
+
+		$result = $this->trait->getTestUrls($url, $removeWebRoot);
+		$this->assertEquals($expected, $result);
+	}
+
+	public static function dataGetTestUrls(): array {
+		return [
+			'same cli and base URL' => [
+				'/apps/files/js/example.js', false, 'https://nextcloud.local', '', ['nextcloud.local'], [
+					// from cli url
+					'https://nextcloud.local/apps/files/js/example.js',
+					// http variant from trusted domains
+					'http://nextcloud.local/apps/files/js/example.js',
+				]
+			],
+			'different cli and base URL' => [
+				'/apps/files/js/example.js', false, 'https://example.com', '', ['nextcloud.local'], [
+					// from cli url
+					'https://example.com/apps/files/js/example.js',
+					// from base url
+					'https://nextcloud.local/apps/files/js/example.js',
+					// http variant from trusted domains
+					'http://nextcloud.local/apps/files/js/example.js',
+				]
+			],
+			'different cli and base URL and trusted domains' => [
+				'/apps/files/js/example.js', false, 'https://example.com', '', ['nextcloud.local', 'example.com', '127.0.0.1'], [
+					// from cli url
+					'https://example.com/apps/files/js/example.js',
+					// from base url
+					'https://nextcloud.local/apps/files/js/example.js',
+					// http variant from trusted domains
+					'http://nextcloud.local/apps/files/js/example.js',
+					'http://example.com/apps/files/js/example.js',
+					// trusted domains
+					'https://127.0.0.1/apps/files/js/example.js',
+					'http://127.0.0.1/apps/files/js/example.js',
+				]
+			],
+			'wildcard trusted domains' => [
+				'/apps/files/js/example.js', false, '', '', ['nextcloud.local', '*.example.com'], [
+					// from base url
+					'https://nextcloud.local/apps/files/js/example.js',
+					// http variant from trusted domains
+					'http://nextcloud.local/apps/files/js/example.js',
+					// trusted domains with wild card are skipped
+				]
+			],
+			'missing leading slash' => [
+				'apps/files/js/example.js', false, 'https://nextcloud.local', '', ['nextcloud.local'], [
+					// from cli url
+					'https://nextcloud.local/apps/files/js/example.js',
+					// http variant from trusted domains
+					'http://nextcloud.local/apps/files/js/example.js',
+				]
+			],
+			'keep web-root' => [
+				'/apps/files/js/example.js', false, 'https://example.com', '/nextcloud', ['nextcloud.local', 'example.com', '192.168.100.1'], [
+					// from cli url (note that the CLI url has NO web root)
+					'https://example.com/apps/files/js/example.js',
+					// from base url
+					'https://nextcloud.local/nextcloud/apps/files/js/example.js',
+					// http variant from trusted domains
+					'http://nextcloud.local/nextcloud/apps/files/js/example.js',
+					// trusted domains with web-root
+					'https://example.com/nextcloud/apps/files/js/example.js',
+					'http://example.com/nextcloud/apps/files/js/example.js',
+					'https://192.168.100.1/nextcloud/apps/files/js/example.js',
+					'http://192.168.100.1/nextcloud/apps/files/js/example.js',
+				]
+			],
+			// example if the URL is generated by the URL generator
+			'keep web-root and web root in url' => [
+				'/nextcloud/apps/files/js/example.js', false, 'https://example.com', '/nextcloud', ['nextcloud.local', 'example.com', '192.168.100.1'], [
+					// from cli url (note that the CLI url has NO web root)
+					'https://example.com/apps/files/js/example.js',
+					// from base url
+					'https://nextcloud.local/nextcloud/apps/files/js/example.js',
+					// http variant from trusted domains
+					'http://nextcloud.local/nextcloud/apps/files/js/example.js',
+					// trusted domains with web-root
+					'https://example.com/nextcloud/apps/files/js/example.js',
+					'http://example.com/nextcloud/apps/files/js/example.js',
+					'https://192.168.100.1/nextcloud/apps/files/js/example.js',
+					'http://192.168.100.1/nextcloud/apps/files/js/example.js',
+				]
+			],
+			'remove web-root' => [
+				'/.well-known/caldav', true, 'https://example.com', '/nextcloud', ['nextcloud.local', 'example.com', '192.168.100.1'], [
+					// from cli url (note that the CLI url has NO web root)
+					'https://example.com/.well-known/caldav',
+					// from base url
+					'https://nextcloud.local/.well-known/caldav',
+					// http variant from trusted domains
+					'http://nextcloud.local/.well-known/caldav',
+					'http://example.com/.well-known/caldav',
+					// trusted domains with web-root
+					'https://192.168.100.1/.well-known/caldav',
+					'http://192.168.100.1/.well-known/caldav',
+				]
+			],
+			// example if the URL is generated by the URL generator
+			'remove web-root and web root in url' => [
+				'/nextcloud/.well-known/caldav', true, 'https://example.com', '/nextcloud', ['nextcloud.local', 'example.com', '192.168.100.1'], [
+					// from cli url (note that the CLI url has NO web root)
+					'https://example.com/.well-known/caldav',
+					// from base url
+					'https://nextcloud.local/.well-known/caldav',
+					// http variant from trusted domains
+					'http://nextcloud.local/.well-known/caldav',
+					'http://example.com/.well-known/caldav',
+					// trusted domains with web-root
+					'https://192.168.100.1/.well-known/caldav',
+					'http://192.168.100.1/.well-known/caldav',
+				]
+			],
+		];
+	}
+
+}
diff --git a/lib/base.php b/lib/base.php
@@ -104,7 +104,7 @@ class OC {
 	 */
 	private static string $SUBURI = '';
 	/**
-	 * the Nextcloud root path for http requests (e.g. nextcloud/)
+	 * the Nextcloud root path for http requests (e.g. /nextcloud)
 	 */
 	public static string $WEBROOT = '';
 	/**