Skip to content

Commit d126fc3

Browse files
susnuxcome-nc
susnux
and
come-nc
committed
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>
1 parent 39d66ed commit d126fc3

File tree

5 files changed

+74
-69
lines changed

5 files changed

+74
-69
lines changed

‎apps/settings/lib/SetupChecks/CheckServerResponseTrait.php‎

Lines changed: 41 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -54,36 +54,38 @@ protected function serverConfigHelp(): string {
5454
* Get all possible URLs that need to be checked for a local request test.
5555
* This takes all `trusted_domains` and the CLI overwrite URL into account.
5656
*
57-
* @param string $url The relative URL to test starting with a /
57+
* @param string $url The absolute path (absolute URL without host but with web-root) to test starting with a /
58+
* @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')
5859
* @return list<string> List of possible absolute URLs
5960
*/
60-
protected function getTestUrls(string $url, bool $removeWebroot): array {
61+
protected function getTestUrls(string $url, bool $isRootRequest = false): array {
6162
$url = '/' . ltrim($url, '/');
6263

6364
$webroot = rtrim($this->urlGenerator->getWebroot(), '/');
64-
// Similar to `getAbsoluteURL` of URLGenerator:
65-
// The Nextcloud web root could already be prepended.
66-
if ($webroot !== '' && str_starts_with($url, $webroot)) {
65+
if ($isRootRequest === false && $webroot !== '' && str_starts_with($url, $webroot)) {
66+
// The URL contains the web-root but also the base url does so,
67+
// so we need to remove the web-root from the URL.
6768
$url = substr($url, strlen($webroot));
6869
}
6970

70-
$hosts = [];
71+
// Base URLs to test
72+
$baseUrls = [];
7173

72-
/* Try overwrite.cli.url first, it’s supposed to be how the server contacts itself */
74+
// Try overwrite.cli.url first, it’s supposed to be how the server contacts itself
7375
$cliUrl = $this->config->getSystemValueString('overwrite.cli.url', '');
7476
if ($cliUrl !== '') {
75-
$hosts[] = $this->normalizeUrl(
77+
// The CLI URL already contains the web-root, so we need to normalize it if requested
78+
$baseUrls[] = $this->normalizeUrl(
7679
$cliUrl,
77-
$webroot,
78-
$removeWebroot
80+
$isRootRequest
7981
);
8082
}
8183

82-
/* Try URL generator second */
83-
$hosts[] = $this->normalizeUrl(
84+
// Try URL generator second
85+
// The base URL also contains the webroot so also normalize it
86+
$baseUrls[] = $this->normalizeUrl(
8487
$this->urlGenerator->getBaseUrl(),
85-
$webroot,
86-
$removeWebroot
88+
$isRootRequest
8789
);
8890

8991
/* Last resort: trusted domains */
@@ -93,47 +95,52 @@ protected function getTestUrls(string $url, bool $removeWebroot): array {
9395
/* Ignore domains with a wildcard */
9496
continue;
9597
}
96-
$hosts[] = $this->normalizeUrl("https://$host$webroot", $webroot, $removeWebroot);
97-
$hosts[] = $this->normalizeUrl("http://$host$webroot", $webroot, $removeWebroot);
98+
$baseUrls[] = $this->normalizeUrl("https://$host$webroot", $isRootRequest);
99+
$baseUrls[] = $this->normalizeUrl("http://$host$webroot", $isRootRequest);
98100
}
99101

100-
return array_map(fn (string $host) => $host . $url, array_values(array_unique($hosts)));
102+
return array_map(fn (string $host) => $host . $url, array_values(array_unique($baseUrls)));
101103
}
102104

103105
/**
104106
* Strip a trailing slash and remove the webroot if requested.
107+
* @param string $url The URL to normalize. Should be an absolute URL containing scheme, host and optionally web-root.
108+
* @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
105109
*/
106-
protected function normalizeUrl(string $url, string $webroot, bool $removeWebroot): string {
107-
$url = rtrim($url, '/');
108-
if ($removeWebroot && $webroot !== '' && str_ends_with($url, $webroot)) {
109-
$url = substr($url, 0, -strlen($webroot));
110+
protected function normalizeUrl(string $url, bool $removeWebroot): string {
111+
if ($removeWebroot) {
112+
$segments = parse_url($url);
113+
$port = isset($segments['port']) ? (':' . $segments['port']) : '';
114+
return $segments['scheme'] . '://' . $segments['host'] . $port;
110115
}
111116
return rtrim($url, '/');
112117
}
113118

114119
/**
115120
* Run a HTTP request to check header
116121
* @param string $method The HTTP method to use
117-
* @param string $url The relative URL to check (e.g. output of IURLGenerator)
118-
* @param bool $removeWebroot Remove the webroot from the URL (handle URL as relative to domain root)
119-
* @param array{ignoreSSL?: bool, httpErrors?: bool, options?: array} $options Additional options, like
120-
* [
121-
* // Ignore invalid SSL certificates (e.g. self signed)
122-
* 'ignoreSSL' => true,
123-
* // Ignore requests with HTTP errors (will not yield if request has a 4xx or 5xx response)
124-
* 'httpErrors' => true,
125-
* ]
122+
* @param string $url The absolute path (URL with webroot but without host) to check, can be the output of `IURLGenerator`
123+
* @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.
124+
* @param array{ignoreSSL?: bool, httpErrors?: bool, options?: array} $options HTTP client related options, like
125+
* [
126+
* // Ignore invalid SSL certificates (e.g. self signed)
127+
* 'ignoreSSL' => true,
128+
* // Ignore requests with HTTP errors (will not yield if request has a 4xx or 5xx response)
129+
* 'httpErrors' => true,
130+
* // Additional options for the HTTP client (see `IClient`)
131+
* 'options' => [],
132+
* ]
126133
*
127134
* @return Generator<int, IResponse>
128135
*/
129-
protected function runRequest(string $method, string $url, array $options = [], bool $removeWebroot = false): Generator {
136+
protected function runRequest(string $method, string $url, array $options = [], bool $isRootRequest = false): Generator {
130137
$options = array_merge(['ignoreSSL' => true, 'httpErrors' => true], $options);
131138

132139
$client = $this->clientService->newClient();
133140
$requestOptions = $this->getRequestOptions($options['ignoreSSL'], $options['httpErrors']);
134141
$requestOptions = array_merge($requestOptions, $options['options'] ?? []);
135142

136-
foreach ($this->getTestUrls($url, $removeWebroot) as $testURL) {
143+
foreach ($this->getTestUrls($url, $isRootRequest) as $testURL) {
137144
try {
138145
yield $client->request($method, $testURL, $requestOptions);
139146
} catch (\Throwable $e) {
@@ -147,11 +154,10 @@ protected function runRequest(string $method, string $url, array $options = [],
147154
* @param string $url The relative URL to check (e.g. output of IURLGenerator)
148155
* @param bool $ignoreSSL Ignore SSL certificates
149156
* @param bool $httpErrors Ignore requests with HTTP errors (will not yield if request has a 4xx or 5xx response)
150-
* @param bool $removeWebroot Remove the webroot from the URL (handle URL as relative to domain root)
151157
* @return Generator<int, IResponse>
152158
*/
153-
protected function runHEAD(string $url, bool $ignoreSSL = true, bool $httpErrors = true, bool $removeWebroot = false): Generator {
154-
return $this->runRequest('HEAD', $url, ['ignoreSSL' => $ignoreSSL, 'httpErrors' => $httpErrors], $removeWebroot);
159+
protected function runHEAD(string $url, bool $ignoreSSL = true, bool $httpErrors = true): Generator {
160+
return $this->runRequest('HEAD', $url, ['ignoreSSL' => $ignoreSSL, 'httpErrors' => $httpErrors]);
155161
}
156162

157163
protected function getRequestOptions(bool $ignoreSSL, bool $httpErrors): array {

‎apps/settings/lib/SetupChecks/DataDirectoryProtected.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ public function getName(): string {
5757
}
5858

5959
public function run(): SetupResult {
60-
$datadir = str_replace(\OC::$SERVERROOT . '/', '', $this->config->getSystemValue('datadirectory', ''));
61-
$dataUrl = '/' . $datadir . '/.ocdata';
60+
$dataDir = str_replace(\OC::$SERVERROOT . '/', '', $this->config->getSystemValueString('datadirectory', ''));
61+
$dataUrl = $this->urlGenerator->linkTo('', $dataDir . '/.ocdata');
6262

6363
$noResponse = true;
6464
foreach ($this->runHEAD($dataUrl, httpErrors:false) as $response) {

‎apps/settings/lib/SetupChecks/WellKnownUrls.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public function run(): SetupResult {
7171
$requestOptions = ['httpErrors' => false, 'options' => ['allow_redirects' => ['track_redirects' => true]]];
7272
foreach ($urls as [$verb,$url,$validStatuses,$checkCustomHeader]) {
7373
$works = null;
74-
foreach ($this->runRequest($verb, $url, $requestOptions, removeWebroot: true) as $response) {
74+
foreach ($this->runRequest($verb, $url, $requestOptions, isRootRequest: true) as $response) {
7575
// Check that the response status matches
7676
$works = in_array($response->getStatusCode(), $validStatuses);
7777
// and (if needed) the custom Nextcloud header is set

‎apps/settings/tests/SetupChecks/CheckServerResponseTraitTest.php‎

Lines changed: 27 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ class CheckServerResponseTraitTest extends TestCase {
2020

2121
protected const BASE_URL = 'https://nextcloud.local';
2222

23-
private IL10N&MockObject $l10n;
24-
private IConfig&MockObject $config;
25-
private IURLGenerator&MockObject $urlGenerator;
26-
private IClientService&MockObject $clientService;
27-
private LoggerInterface&MockObject $logger;
23+
private IL10N|MockObject $l10n;
24+
private IConfig|MockObject $config;
25+
private IURLGenerator|MockObject $urlGenerator;
26+
private IClientService|MockObject $clientService;
27+
private LoggerInterface|MockObject $logger;
2828

2929
private CheckServerResponseTraitImplementation $trait;
3030

@@ -51,16 +51,27 @@ protected function setUp(): void {
5151
/**
5252
* @dataProvider dataNormalizeUrl
5353
*/
54-
public function testNormalizeUrl(string $url, string $webRoot, bool $removeWebRoot, string $expected): void {
55-
$this->assertEquals($expected, $this->trait->normalizeUrl($url, $webRoot, $removeWebRoot));
54+
public function testNormalizeUrl(string $url, bool $isRootRequest, string $expected): void {
55+
$this->assertEquals($expected, $this->trait->normalizeUrl($url, $isRootRequest));
5656
}
5757

5858
public static function dataNormalizeUrl(): array {
5959
return [
60-
'valid and nothing to change' => ['http://example.com/root', '/root', false, 'http://example.com/root'],
61-
'trailing slash' => ['http://example.com/root/', '/root', false, 'http://example.com/root'],
62-
'remove web root' => ['http://example.com/root/', '/root', true, 'http://example.com'],
63-
'remove web root but empty' => ['http://example.com', '', true, 'http://example.com'],
60+
// untouched web-root
61+
'valid and nothing to change' => ['http://example.com/root', false, 'http://example.com/root'],
62+
'valid with port and nothing to change' => ['http://example.com:8081/root', false, 'http://example.com:8081/root'],
63+
'trailing slash' => ['http://example.com/root/', false, 'http://example.com/root'],
64+
'deep web root' => ['http://example.com/deep/webroot', false, 'http://example.com/deep/webroot'],
65+
// removal of the web-root
66+
'remove web root' => ['http://example.com/root/', true, 'http://example.com'],
67+
'remove web root but empty' => ['http://example.com', true, 'http://example.com'],
68+
'remove deep web root' => ['http://example.com/deep/webroot', true, 'http://example.com'],
69+
'remove web root with port' => ['http://example.com:8081/root', true, 'http://example.com:8081'],
70+
'remove web root with port but empty' => ['http://example.com:8081', true, 'http://example.com:8081'],
71+
'remove web root from IP' => ['https://127.0.0.1/root', true, 'https://127.0.0.1'],
72+
'remove web root from IP with port' => ['https://127.0.0.1:8080/root', true, 'https://127.0.0.1:8080'],
73+
'remove web root from IPv6' => ['https://[ff02::1]/root', true, 'https://[ff02::1]'],
74+
'remove web root from IPv6 with port' => ['https://[ff02::1]:8080/root', true, 'https://[ff02::1]:8080'],
6475
];
6576
}
6677

@@ -69,7 +80,7 @@ public static function dataNormalizeUrl(): array {
6980
*/
7081
public function testGetTestUrls(
7182
string $url,
72-
bool $removeWebRoot,
83+
bool $isRootRequest,
7384
string $cliUrl,
7485
string $webRoot,
7586
array $trustedDomains,
@@ -93,10 +104,13 @@ public function testGetTestUrls(
93104
->method('getBaseUrl')
94105
->willReturn(self::BASE_URL . $webRoot);
95106

96-
$result = $this->trait->getTestUrls($url, $removeWebRoot);
107+
$result = $this->trait->getTestUrls($url, $isRootRequest);
97108
$this->assertEquals($expected, $result);
98109
}
99110

111+
/**
112+
* @return array<string, list{string, bool, string, string, list<string>, list<string>}>
113+
*/
100114
public static function dataGetTestUrls(): array {
101115
return [
102116
'same cli and base URL' => [
@@ -193,21 +207,6 @@ public static function dataGetTestUrls(): array {
193207
'http://192.168.100.1/.well-known/caldav',
194208
]
195209
],
196-
// example if the URL is generated by the URL generator
197-
'remove web-root and web root in url' => [
198-
'/nextcloud/.well-known/caldav', true, 'https://example.com', '/nextcloud', ['nextcloud.local', 'example.com', '192.168.100.1'], [
199-
// from cli url (note that the CLI url has NO web root)
200-
'https://example.com/.well-known/caldav',
201-
// from base url
202-
'https://nextcloud.local/.well-known/caldav',
203-
// http variant from trusted domains
204-
'http://nextcloud.local/.well-known/caldav',
205-
'http://example.com/.well-known/caldav',
206-
// trusted domains with web-root
207-
'https://192.168.100.1/.well-known/caldav',
208-
'http://192.168.100.1/.well-known/caldav',
209-
]
210-
],
211210
];
212211
}
213212

‎apps/settings/tests/SetupChecks/DataDirectoryProtectedTest.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,14 +90,14 @@ public function testStatusCode(array $status, string $expected): void {
9090

9191
$this->config
9292
->expects($this->once())
93-
->method('getSystemValue')
93+
->method('getSystemValueString')
9494
->willReturn('');
9595

9696
$result = $this->setupcheck->run();
9797
$this->assertEquals($expected, $result->getSeverity());
9898
}
9999

100-
public function dataTestStatusCode(): array {
100+
public static function dataTestStatusCode(): array {
101101
return [
102102
'success: forbidden access' => [[403], SetupResult::SUCCESS],
103103
'error: can access' => [[200], SetupResult::ERROR],
@@ -117,7 +117,7 @@ public function testNoResponse(): void {
117117

118118
$this->config
119119
->expects($this->once())
120-
->method('getSystemValue')
120+
->method('getSystemValueString')
121121
->willReturn('');
122122

123123
$result = $this->setupcheck->run();

0 commit comments

Comments
 (0)