Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Oct 1, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8708164
fix: make federation address book sync work with allow_local_remote_s…
kesselb Sep 27, 2024
6be0043
chore: always execute parse_url in preventLocalAddress
kesselb Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 36 additions & 10 deletions apps/dav/lib/CardDAV/SyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also add 'nextcloud' => ['allow_local_address' => true], to always allow private IP ranges for address book federation. But we should also enable it for the initial handshake then, and therefore that's something for a follow-up.

];

$response = $client->request(
'REPORT',
$addressBookUrl,
$uri,
$options
);

Expand All @@ -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
);

Expand Down
55 changes: 55 additions & 0 deletions apps/dav/tests/unit/CardDAV/SyncServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
];
}
}
9 changes: 5 additions & 4 deletions lib/private/Http/Client/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down
8 changes: 7 additions & 1 deletion tests/lib/Http/Client/ClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand All @@ -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'],
];
Expand Down
Loading