diff --git a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php index d4e0567cfafcb..3080829cb00c4 100644 --- a/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php +++ b/apps/settings/lib/SetupChecks/CheckServerResponseTrait.php @@ -37,59 +37,64 @@ 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 / - * @return string[] List of possible absolute URLs + * @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 List of possible absolute URLs */ - protected function getTestUrls(string $url, bool $removeWebroot): array { - $testUrls = []; + protected function getTestUrls(string $url, bool $isRootRequest = false): array { + $url = '/' . ltrim($url, '/'); $webroot = rtrim($this->urlGenerator->getWebroot(), '/'); + 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)); + } - /* Try overwrite.cli.url first, it’s supposed to be how the server contacts itself */ - $cliUrl = $this->config->getSystemValueString('overwrite.cli.url', ''); + // Base URLs to test + $baseUrls = []; + // 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( + // The CLI URL already contains the web-root, so we need to normalize it if requested + $baseUrls[] = $this->normalizeUrl( $cliUrl, - $webroot, - $removeWebroot + $isRootRequest ); - - $testUrls[] = $cliUrl . $url; } - /* Try URL generator second */ - $baseUrl = $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 ); - 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; + $baseUrls[] = $this->normalizeUrl("https://$host$webroot", $isRootRequest); + $baseUrls[] = $this->normalizeUrl("http://$host$webroot", $isRootRequest); } - return $testUrls; + 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 && str_ends_with($url, $webroot)) { - $url = substr($url, -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, '/'); } @@ -97,25 +102,28 @@ protected function normalizeUrl(string $url, string $webroot, bool $removeWebroo /** * Run a HTTP request to check header * @param string $method The HTTP method to use - * @param string $url The relative URL to check - * @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 */ - 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) { @@ -126,7 +134,7 @@ 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) * @return Generator diff --git a/apps/settings/lib/SetupChecks/DataDirectoryProtected.php b/apps/settings/lib/SetupChecks/DataDirectoryProtected.php index 447c66d5c5de4..051494adb6258 100644 --- a/apps/settings/lib/SetupChecks/DataDirectoryProtected.php +++ b/apps/settings/lib/SetupChecks/DataDirectoryProtected.php @@ -40,9 +40,8 @@ public function getName(): string { } public function run(): SetupResult { - $datadir = str_replace(\OC::$SERVERROOT . '/', '', $this->config->getSystemValue('datadirectory', '')); - - $dataUrl = $this->urlGenerator->getWebroot() . '/' . $datadir . '/.ncdata'; + $dataDir = str_replace(\OC::$SERVERROOT . '/', '', $this->config->getSystemValueString('datadirectory', '')); + $dataUrl = $this->urlGenerator->linkTo('', $dataDir . '/.ncdata'); $noResponse = true; foreach ($this->runRequest('GET', $dataUrl, [ 'httpErrors' => false ]) as $response) { diff --git a/apps/settings/lib/SetupChecks/WellKnownUrls.php b/apps/settings/lib/SetupChecks/WellKnownUrls.php index 565544cfdd790..623b9fae90c6e 100644 --- a/apps/settings/lib/SetupChecks/WellKnownUrls.php +++ b/apps/settings/lib/SetupChecks/WellKnownUrls.php @@ -50,9 +50,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, 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/CheckServerResponseTraitImplementation.php b/apps/settings/tests/SetupChecks/CheckServerResponseTraitImplementation.php new file mode 100644 index 0000000000000..6c8b65855cc93 --- /dev/null +++ b/apps/settings/tests/SetupChecks/CheckServerResponseTraitImplementation.php @@ -0,0 +1,40 @@ +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, bool $isRootRequest, string $expected): void { + $this->assertEquals($expected, $this->trait->normalizeUrl($url, $isRootRequest)); + } + + public static function dataNormalizeUrl(): array { + return [ + // 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'], + ]; + } + + /** + * @dataProvider dataGetTestUrls + */ + public function testGetTestUrls( + string $url, + bool $isRootRequest, + 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, $isRootRequest); + $this->assertEquals($expected, $result); + } + + /** + * @return array, list}> + */ + 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', + ] + ], + ]; + } + +} diff --git a/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php b/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php index fbee52fcf0f08..51dffe58787c9 100644 --- a/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php +++ b/apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php @@ -20,17 +20,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()) @@ -74,14 +74,14 @@ public function testStatusCode(array $status, string $expected, bool $hasBody): $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, true], 'success: forbidden access with redirect' => [[200], SetupResult::SUCCESS, false], @@ -102,7 +102,7 @@ public function testNoResponse(): void { $this->config ->expects($this->once()) - ->method('getSystemValue') + ->method('getSystemValueString') ->willReturn(''); $result = $this->setupcheck->run(); diff --git a/lib/base.php b/lib/base.php index 4e66d389e7b12..7654aef2b6092 100644 --- a/lib/base.php +++ b/lib/base.php @@ -43,7 +43,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 = ''; /** diff --git a/lib/private/URLGenerator.php b/lib/private/URLGenerator.php index ab568a3296d5a..39b825e427b01 100644 --- a/lib/private/URLGenerator.php +++ b/lib/private/URLGenerator.php @@ -245,7 +245,7 @@ public function imagePath(string $appName, string $file): string { /** * Makes an URL absolute - * @param string $url the url in the ownCloud host + * @param string $url the url in the Nextcloud host * @return string the absolute version of the url */ public function getAbsoluteURL(string $url): string { @@ -254,7 +254,7 @@ public function getAbsoluteURL(string $url): string { if (\OC::$CLI && !\defined('PHPUNIT_RUN')) { return rtrim($this->config->getSystemValueString('overwrite.cli.url'), '/') . '/' . ltrim($url, '/'); } - // The ownCloud web root can already be prepended. + // The Nextcloud web root could already be prepended. if (\OC::$WEBROOT !== '' && str_starts_with($url, \OC::$WEBROOT)) { $url = substr($url, \strlen(\OC::$WEBROOT)); }