From 870816466f2d1adaf956a83491c0645556b0d02b Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Fri, 27 Sep 2024 19:16:59 +0200 Subject: [PATCH 1/2] fix: make federation address book sync work with allow_local_remote_servers = false Client.preventLocalAddress expects an absolute URL, which means the base_uri option cannot be used. Signed-off-by: Daniel Kesselberg --- apps/dav/lib/CardDAV/SyncService.php | 46 ++++++++++++---- .../tests/unit/CardDAV/SyncServiceTest.php | 55 +++++++++++++++++++ 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index 5f8752a9d492c..05a08d08286ba 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -108,25 +108,54 @@ public function ensureSystemAddressBookExists(string $principal, string $uri, ar }, $this->dbConnection); } + private function prepareUri(string $host, string $path): string { + /* + * The trailing slash is important for merging the uris together. + * + * $host is stored in oc_trusted_servers.url and usually without a trailing slash. + * + * Example for a report request + * + * $host = 'https://server.internal/cloud' + * $path = 'remote.php/dav/addressbooks/system/system/system' + * + * Without the trailing slash, the webroot is missing: + * https://server.internal/remote.php/dav/addressbooks/system/system/system + * + * Example for a download request + * + * $host = 'https://server.internal/cloud' + * $path = '/cloud/remote.php/dav/addressbooks/system/system/system/Database:alice.vcf' + * + * The response from the remote usually contains the webroot already and must be normalized to: + * https://server.internal/cloud/remote.php/dav/addressbooks/system/system/system/Database:alice.vcf + */ + $host = rtrim($host, '/') . '/'; + + $uri = \GuzzleHttp\Psr7\UriResolver::resolve( + \GuzzleHttp\Psr7\Utils::uriFor($host), + \GuzzleHttp\Psr7\Utils::uriFor($path) + ); + + return (string)$uri; + } + /** * @throws ClientExceptionInterface */ protected function requestSyncReport(string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken): array { $client = $this->clientService->newClient(); - - // the trailing slash is important for merging base_uri and uri - $url = rtrim($url, '/') . '/'; + $uri = $this->prepareUri($url, $addressBookUrl); $options = [ 'auth' => [$userName, $sharedSecret], - 'base_uri' => $url, 'body' => $this->buildSyncCollectionRequestBody($syncToken), 'headers' => ['Content-Type' => 'application/xml'] ]; $response = $client->request( 'REPORT', - $addressBookUrl, + $uri, $options ); @@ -138,17 +167,14 @@ protected function requestSyncReport(string $url, string $userName, string $addr protected function download(string $url, string $userName, string $sharedSecret, string $resourcePath): string { $client = $this->clientService->newClient(); - - // the trailing slash is important for merging base_uri and uri - $url = rtrim($url, '/') . '/'; + $uri = $this->prepareUri($url, $resourcePath); $options = [ 'auth' => [$userName, $sharedSecret], - 'base_uri' => $url, ]; $response = $client->get( - $resourcePath, + $uri, $options ); diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php index 570c55eab0540..db99f73306d26 100644 --- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php +++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php @@ -426,4 +426,59 @@ public function testDeleteAddressbookWhenAccessRevoked(): void { [] ); } + + /** + * @dataProvider providerUseAbsoluteUriReport + */ + public function testUseAbsoluteUriReport(string $host, string $expected): void { + $body = ' + + http://sabre.io/ns/sync/1 +'; + + $requestResponse = new Response(new PsrResponse( + 207, + ['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)], + $body + )); + + $this->client + ->method('request') + ->with( + 'REPORT', + $this->callback(function ($uri) use ($expected) { + $this->assertEquals($expected, $uri); + return true; + }), + $this->callback(function ($options) { + $this->assertIsArray($options); + return true; + }), + ) + ->willReturn($requestResponse); + + $this->service->syncRemoteAddressBook( + $host, + 'system', + 'remote.php/dav/addressbooks/system/system/system', + '1234567890', + null, + '1', + 'principals/system/system', + [] + ); + } + + public function providerUseAbsoluteUriReport(): array { + return [ + ['https://server.internal', 'https://server.internal/remote.php/dav/addressbooks/system/system/system'], + ['https://server.internal/', 'https://server.internal/remote.php/dav/addressbooks/system/system/system'], + ['https://server.internal/nextcloud', 'https://server.internal/nextcloud/remote.php/dav/addressbooks/system/system/system'], + ['https://server.internal/nextcloud/', 'https://server.internal/nextcloud/remote.php/dav/addressbooks/system/system/system'], + ['https://server.internal:8080', 'https://server.internal:8080/remote.php/dav/addressbooks/system/system/system'], + ['https://server.internal:8080/', 'https://server.internal:8080/remote.php/dav/addressbooks/system/system/system'], + ['https://server.internal:8080/nextcloud', 'https://server.internal:8080/nextcloud/remote.php/dav/addressbooks/system/system/system'], + ['https://server.internal:8080/nextcloud/', 'https://server.internal:8080/nextcloud/remote.php/dav/addressbooks/system/system/system'], + ]; + } } From 6be00432b75a80a246246883c5fa955ce803f3d8 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Mon, 30 Sep 2024 13:05:19 +0200 Subject: [PATCH 2/2] chore: always execute parse_url in preventLocalAddress This change should make it easier to spot wrong uses of the HTTP client on development setups where allow_local_remote_servers is usually true. Signed-off-by: Daniel Kesselberg --- lib/private/Http/Client/Client.php | 9 +++++---- tests/lib/Http/Client/ClientTest.php | 8 +++++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index 40ce012cd1a0d..62209ff9040d1 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -158,14 +158,15 @@ private function isLocalAddressAllowed(array $options) : bool { } protected function preventLocalAddress(string $uri, array $options): void { - if ($this->isLocalAddressAllowed($options)) { - return; - } - $host = parse_url($uri, PHP_URL_HOST); if ($host === false || $host === null) { throw new LocalServerException('Could not detect any host'); } + + if ($this->isLocalAddressAllowed($options)) { + return; + } + if (!$this->remoteHostValidator->isValid($host)) { throw new LocalServerException('Host "' . $host . '" violates local access rules'); } diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index 237bb1299e526..47a6b885aed09 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -130,6 +130,13 @@ public function testGetProxyUriProxyHostWithPasswordAndExclude(): void { ], self::invokePrivate($this->client, 'getProxyUri')); } + public function testPreventLocalAddressThrowOnInvalidUri(): void { + $this->expectException(LocalServerException::class); + $this->expectExceptionMessage('Could not detect any host'); + + self::invokePrivate($this->client, 'preventLocalAddress', ['!@#$', []]); + } + public function dataPreventLocalAddress():array { return [ ['https://localhost/foo.bar'], @@ -146,7 +153,6 @@ public function dataPreventLocalAddress():array { ['https://10.0.0.1'], ['https://another-host.local'], ['https://service.localhost'], - ['!@#$', true], // test invalid url ['https://normal.host.com'], ['https://com.one-.nextcloud-one.com'], ];