Merge pull request #48451 from nextcloud/bug/noid/federated-addressbook-sync-without-localaddressallowed

fix: make federation address book sync work with allow_local_remote_servers = false

Author: kesselb

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
 		);
 

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 = '<?xml version="1.0"?>
+<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:card="urn:ietf:params:xml:ns:carddav" xmlns:oc="http://owncloud.org/ns">
+    <d:sync-token>http://sabre.io/ns/sync/1</d:sync-token>
+</d:multistatus>';
+
+		$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'],
+		];
+	}
 }

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');
 		}

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'],
 		];