refactor(settings): CheckServerResponseTrait always expect absolute…

… path Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de> Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com> Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
nextcloud · Oct 11, 2024 · 6b964d8 · 6b964d8
1 parent 39d66ed
commit 6b964d8
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 71 deletions.
diff --git a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php
@@ -54,36 +54,38 @@ 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 starting with a /
+	 * @param string $url The absolute path (absolute URL without host but with web-root) to test starting with a /
+	 * @param bool $isRootRequest Set to remove the web-root from URL and host (e.g. when requesting a path in the domain root like '/.well-known')
 	 * @return list<string> List of possible absolute URLs
 	 */
-	protected function getTestUrls(string $url, bool $removeWebroot): array {
+	protected function getTestUrls(string $url, bool $isRootRequest = false): array {
 		$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)) {
+		if ($isRootRequest === false && $webroot !== '' && str_starts_with($url, $webroot)) {
+			// The URL contains the web-root but also the base url does so,
+			// so we need to remove the web-root from the URL.
 			$url = substr($url, strlen($webroot));
 		}
 
-		$hosts = [];
+		// Base URLs to test
+		$baseUrls = [];
 
-		/* Try overwrite.cli.url first, it’s supposed to be how the server contacts itself */
+		// Try overwrite.cli.url first, it’s supposed to be how the server contacts itself
 		$cliUrl = $this->config->getSystemValueString('overwrite.cli.url', '');
 		if ($cliUrl !== '') {
-			$hosts[] = $this->normalizeUrl(
+			// The CLI URL already contains the web-root, so we need to normalize it if requested
+			$baseUrls[] = $this->normalizeUrl(
 				$cliUrl,
-				$webroot,
-				$removeWebroot
+				$isRootRequest
 			);
 		}
 
-		/* Try URL generator second */
-		$hosts[] = $this->normalizeUrl(
+		// Try URL generator second
+		// The base URL also contains the webroot so also normalize it
+		$baseUrls[] = $this->normalizeUrl(
 			$this->urlGenerator->getBaseUrl(),
-			$webroot,
-			$removeWebroot
+			$isRootRequest
 		);
 
 		/* Last resort: trusted domains */
@@ -93,47 +95,52 @@ protected function getTestUrls(string $url, bool $removeWebroot): array {
 				/* Ignore domains with a wildcard */
 				continue;
 			}
-			$hosts[] = $this->normalizeUrl("https://$host$webroot", $webroot, $removeWebroot);
-			$hosts[] = $this->normalizeUrl("http://$host$webroot", $webroot, $removeWebroot);
+			$baseUrls[] = $this->normalizeUrl("https://$host$webroot", $isRootRequest);
+			$baseUrls[] = $this->normalizeUrl("http://$host$webroot", $isRootRequest);
 		}
 
-		return array_map(fn (string $host) => $host . $url, array_values(array_unique($hosts)));
+		return array_map(fn (string $host) => $host . $url, array_values(array_unique($baseUrls)));
 	}
 
 	/**
 	 * Strip a trailing slash and remove the webroot if requested.
+	 * @param string $url The URL to normalize. Should be an absolute URL containing scheme, host and optionally web-root.
+	 * @param bool $removeWebroot If set the web-root is removed from the URL and an absolute URL with only the scheme and host (optional port) is returned
 	 */
-	protected function normalizeUrl(string $url, string $webroot, bool $removeWebroot): string {
-		$url = rtrim($url, '/');
-		if ($removeWebroot && $webroot !== '' && str_ends_with($url, $webroot)) {
-			$url = substr($url, 0, -strlen($webroot));
+	protected function normalizeUrl(string $url, bool $removeWebroot): string {
+		if ($removeWebroot) {
+			$segments = parse_url($url);
+			$port = isset($segments['port']) ? (':' . $segments['port']) : '';
+			return $segments['scheme'] . '://' . $segments['host'] . $port;
 		}
 		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 (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)
-	 *                                                  'ignoreSSL' => true,
-	 *                                                  // Ignore requests with HTTP errors (will not yield if request has a 4xx or 5xx response)
-	 *                                                  'httpErrors' => true,
-	 *                                                 ]
+	 * @param string $url The absolute path (URL with webroot but without host) to check, can be the output of `IURLGenerator`
+	 * @param bool $isRootRequest If set the webroot is removed from URLs to make the request target the host's root. Example usage are the /.well-known URLs in the root path.
+	 * @param array{ignoreSSL?: bool, httpErrors?: bool, options?: array} $options HTTP client related options, like
+	 *                                                                             [
+	 *                                                                             // Ignore invalid SSL certificates (e.g. self signed)
+	 *                                                                             'ignoreSSL' => true,
+	 *                                                                             // Ignore requests with HTTP errors (will not yield if request has a 4xx or 5xx response)
+	 *                                                                             'httpErrors' => true,
+	 *                                                                             // Additional options for the HTTP client (see `IClient`)
+	 *                                                                             'options' => [],
+	 *                                                                             ]
 	 *
 	 * @return Generator<int, IResponse>
 	 */
-	protected function runRequest(string $method, string $url, array $options = [], bool $removeWebroot = false): Generator {
+	protected function runRequest(string $method, string $url, array $options = [], bool $isRootRequest = 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, $removeWebroot) as $testURL) {
+		foreach ($this->getTestUrls($url, $isRootRequest) as $testURL) {
 			try {
 				yield $client->request($method, $testURL, $requestOptions);
 			} catch (\Throwable $e) {
@@ -147,11 +154,10 @@ protected function runRequest(string $method, string $url, array $options = [],
 	 * @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, bool $removeWebroot = false): Generator {
-		return $this->runRequest('HEAD', $url, ['ignoreSSL' => $ignoreSSL, 'httpErrors' => $httpErrors], $removeWebroot);
+	protected function runHEAD(string $url, bool $ignoreSSL = true, bool $httpErrors = true): Generator {
+		return $this->runRequest('HEAD', $url, ['ignoreSSL' => $ignoreSSL, 'httpErrors' => $httpErrors]);
 	}
 
 	protected function getRequestOptions(bool $ignoreSSL, bool $httpErrors): array {

diff --git a/apps/settings/lib/SetupChecks/DataDirectoryProtected.php b/apps/settings/lib/SetupChecks/DataDirectoryProtected.php
@@ -57,8 +57,8 @@ public function getName(): string {
 	}
 
 	public function run(): SetupResult {
-		$datadir = str_replace(\OC::$SERVERROOT . '/', '', $this->config->getSystemValue('datadirectory', ''));
-		$dataUrl = '/' . $datadir . '/.ocdata';
+		$dataDir = str_replace(\OC::$SERVERROOT . '/', '', $this->config->getSystemValueString('datadirectory', ''));
+		$dataUrl = $this->urlGenerator->linkTo('', $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
@@ -71,7 +71,7 @@ public function run(): SetupResult {
 		$requestOptions = ['httpErrors' => false, 'options' => ['allow_redirects' => ['track_redirects' => true]]];
 		foreach ($urls as [$verb,$url,$validStatuses,$checkCustomHeader]) {
 			$works = null;
-			foreach ($this->runRequest($verb, $url, $requestOptions, removeWebroot: true) as $response) {
+			foreach ($this->runRequest($verb, $url, $requestOptions, isRootRequest: 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/CheckServerResponseTraitTest.php b/apps/settings/tests/SetupChecks/CheckServerResponseTraitTest.php
@@ -51,16 +51,27 @@ protected function setUp(): void {
 	/**
 	 * @dataProvider dataNormalizeUrl
 	 */
-	public function testNormalizeUrl(string $url, string $webRoot, bool $removeWebRoot, string $expected): void {
-		$this->assertEquals($expected, $this->trait->normalizeUrl($url, $webRoot, $removeWebRoot));
+	public function testNormalizeUrl(string $url, bool $isRootRequest, string $expected): void {
+		$this->assertEquals($expected, $this->trait->normalizeUrl($url, $isRootRequest));
 	}
 
 	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'],
+			// untouched web-root
+			'valid and nothing to change' => ['http://example.com/root', false, 'http://example.com/root'],
+			'valid with port and nothing to change' => ['http://example.com:8081/root', false, 'http://example.com:8081/root'],
+			'trailing slash' => ['http://example.com/root/', false, 'http://example.com/root'],
+			'deep web root' => ['http://example.com/deep/webroot', false, 'http://example.com/deep/webroot'],
+			// removal of the web-root
+			'remove web root' => ['http://example.com/root/', true, 'http://example.com'],
+			'remove web root but empty' => ['http://example.com', true, 'http://example.com'],
+			'remove deep web root' => ['http://example.com/deep/webroot', true, 'http://example.com'],
+			'remove web root with port' => ['http://example.com:8081/root', true, 'http://example.com:8081'],
+			'remove web root with port but empty' => ['http://example.com:8081', true, 'http://example.com:8081'],
+			'remove web root from IP' => ['https://127.0.0.1/root', true, 'https://127.0.0.1'],
+			'remove web root from IP with port' => ['https://127.0.0.1:8080/root', true, 'https://127.0.0.1:8080'],
+			'remove web root from IPv6' => ['https://[ff02::1]/root', true, 'https://[ff02::1]'],
+			'remove web root from IPv6 with port' => ['https://[ff02::1]:8080/root', true, 'https://[ff02::1]:8080'],
 		];
 	}
 
@@ -69,7 +80,7 @@ public static function dataNormalizeUrl(): array {
 	 */
 	public function testGetTestUrls(
 		string $url,
-		bool $removeWebRoot,
+		bool $isRootRequest,
 		string $cliUrl,
 		string $webRoot,
 		array $trustedDomains,
@@ -93,10 +104,13 @@ public function testGetTestUrls(
 			->method('getBaseUrl')
 			->willReturn(self::BASE_URL . $webRoot);
 
-		$result = $this->trait->getTestUrls($url, $removeWebRoot);
+		$result = $this->trait->getTestUrls($url, $isRootRequest);
 		$this->assertEquals($expected, $result);
 	}
 
+	/**
+	 * @return array<string, list{string, bool, string, string, list<string>, list<string>}>
+	 */
 	public static function dataGetTestUrls(): array {
 		return [
 			'same cli and base URL' => [
@@ -193,21 +207,6 @@ public static function dataGetTestUrls(): array {
 					'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/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php b/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php
@@ -37,17 +37,17 @@
 use Test\TestCase;
 
 class DataDirectoryProtectedTest extends TestCase {
-	private IL10N|MockObject $l10n;
-	private IConfig|MockObject $config;
-	private IURLGenerator|MockObject $urlGenerator;
-	private IClientService|MockObject $clientService;
-	private LoggerInterface|MockObject $logger;
-	private DataDirectoryProtected|MockObject $setupcheck;
+	private IL10N&MockObject $l10n;
+	private IConfig&MockObject $config;
+	private IURLGenerator&MockObject $urlGenerator;
+	private IClientService&MockObject $clientService;
+	private LoggerInterface&MockObject $logger;
+	private DataDirectoryProtected&MockObject $setupcheck;
 
 	protected function setUp(): void {
 		parent::setUp();
 
-		/** @var IL10N|MockObject */
+		/** @var IL10N&MockObject */
 		$this->l10n = $this->getMockBuilder(IL10N::class)
 			->disableOriginalConstructor()->getMock();
 		$this->l10n->expects($this->any())
@@ -90,14 +90,14 @@ public function testStatusCode(array $status, string $expected): void {
 
 		$this->config
 			->expects($this->once())
-			->method('getSystemValue')
+			->method('getSystemValueString')
 			->willReturn('');
 
 		$result = $this->setupcheck->run();
 		$this->assertEquals($expected, $result->getSeverity());
 	}
 
-	public function dataTestStatusCode(): array {
+	public static function dataTestStatusCode(): array {
 		return [
 			'success: forbidden access' => [[403], SetupResult::SUCCESS],
 			'error: can access' => [[200], SetupResult::ERROR],
@@ -117,7 +117,7 @@ public function testNoResponse(): void {
 
 		$this->config
 			->expects($this->once())
-			->method('getSystemValue')
+			->method('getSystemValueString')
 			->willReturn('');
 
 		$result = $this->setupcheck->run();