Skip to content

fix: block local remote shares on typos and contacts with a local Cloud ID #64318

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

Draft
salmart-dev wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

fix: block local remote shares on typos and contacts with a local Cloud ID #64318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a4dd0e
fix: tighten offering local users as remote share targets
salmart-dev Sep 14, 2026
2eacff9
fix: stop offering contacts with local cloud ID as remote share targets
salmart-dev Sep 14, 2026
3771454
test: add tests for RemotePlugin changes
salmart-dev Sep 14, 2026
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
61 changes: 50 additions & 11 deletions lib/private/Collaboration/Collaborators/RemotePlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use OCP\Federation\ICloudIdManager;
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\Share\IShare;
Expand Down Expand Up @@ -73,16 +74,21 @@ public function search(string $search, int $limit, int $offset, ISearchResult $s
}

$localUser = $this->userManager->get($remoteUser);
if ($localUser !== null && $remoteUser !== $this->userId && $cloudId === $localUser->getCloudId()) {
$result['wide'][] = [
'label' => $contact['FN'],
'uuid' => $contact['UID'],
'value' => [
'shareType' => IShare::TYPE_USER,
'shareWith' => $remoteUser
],
'shareWithDisplayNameUnique' => $contact['EMAIL'] !== null && $contact['EMAIL'] !== '' ? $contact['EMAIL'] : $contact['UID'],
];
if ($localUser !== null && $this->isLocalUserCloudId($cloudId, $localUser)) {
if ($localUser->getUID() !== $this->userId) {
// add share as local share if not self
$result['wide'][] = [
'label' => $contact['FN'],
'uuid' => $contact['UID'],
'value' => [
'shareType' => IShare::TYPE_USER,
'shareWith' => $localUser->getUID()
],
'shareWithDisplayNameUnique' => $contact['EMAIL'] !== null && $contact['EMAIL'] !== '' ? $contact['EMAIL'] : $contact['UID'],
];
}
// do not offer local user as remote share
continue;
}

$emailMatch = false;
Expand Down Expand Up @@ -139,7 +145,7 @@ public function search(string $search, int $limit, int $offset, ISearchResult $s
try {
[$remoteUser, $serverUrl] = $this->splitUserRemote($search);
$localUser = $this->userManager->get($remoteUser);
if ($localUser === null || $search !== $localUser->getCloudId()) {
if ($localUser === null || !$this->isLocalUserCloudId($search, $localUser)) {
$result['exact'][] = [
'label' => $remoteUser . " ($serverUrl)",
'uuid' => $remoteUser,
Expand All @@ -161,6 +167,39 @@ public function search(string $search, int $limit, int $offset, ISearchResult $s
return true;
}

/**
* Checks if the cloud id points to the given local user, ignoring:
* - letter casing of the user and host
* - protocol
* - trailing slash
*/
private function isLocalUserCloudId(string $cloudId, IUser $localUser): bool {
try {
$remoteCloudId = $this->cloudIdManager->resolveCloudId($cloudId);
$localCloudId = $this->cloudIdManager->resolveCloudId($localUser->getCloudId());
} catch (\InvalidArgumentException) {
return false;
}

return mb_strtolower($remoteCloudId->getUser()) === mb_strtolower($localCloudId->getUser())

@salmart-dev salmart-dev Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A fix in this regard should respect the way that the user backend would actually do comparison for the user ID. According to the OCM specs, the identifier is case-sensitive, so lower-casing for comparison is only okay when the backend would do the same to identify the user. So lower-casing may be okay for checking if an inputted cloud ID may match a local user, but is not okay to compare if two remote users are the same user, as the decision about casing can only be done by the remote.

&& $this->normalizeRemote($remoteCloudId->getRemote()) === $this->normalizeRemote($localCloudId->getRemote());
}

/**
* Normalize a remote for comparison: the protocol and trailing slashes are
* dropped, and the host is lowercased. The path is kept as-is, as it is
* case-sensitive (RFC 3986, section 6.2.2.1).
*/
private function normalizeRemote(string $remote): string {
$remote = rtrim(preg_replace('#^https?://#i', '', $remote), '/');
$pathPosition = strpos($remote, '/');
if ($pathPosition === false) {
return strtolower($remote);
}

return strtolower(substr($remote, 0, $pathPosition)) . substr($remote, $pathPosition);
}
Comment on lines +184 to +201

@provokateurin provokateurin Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This logic should be moved into a new method in ICloudId, so it's reusable.


/**
* split user and remote from federated cloud id
*
Expand Down
149 changes: 146 additions & 3 deletions tests/lib/Collaboration/Collaborators/RemotePluginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\Share\IShare;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;

Expand Down Expand Up @@ -88,7 +89,7 @@ public function instantiatePlugin() {
* @param bool $exactIdMatch
* @param bool $reachedEnd
*/
#[\PHPUnit\Framework\Attributes\DataProvider('dataGetRemote')]
#[DataProvider('dataGetRemote')]
public function testSearch($searchTerm, array $contacts, $shareeEnumeration, array $expected, $exactIdMatch, $reachedEnd): void {
$this->config->expects($this->any())
->method('getAppValue')
Expand Down Expand Up @@ -126,7 +127,7 @@ function ($appName, $key, $default) use ($shareeEnumeration) {
* @param string $expectedUser
* @param string $expectedUrl
*/
#[\PHPUnit\Framework\Attributes\DataProvider('dataTestSplitUserRemote')]
#[DataProvider('dataTestSplitUserRemote')]
public function testSplitUserRemote($remote, $expectedUser, $expectedUrl): void {
$this->instantiatePlugin();

Expand All @@ -142,7 +143,7 @@ public function testSplitUserRemote($remote, $expectedUser, $expectedUrl): void
/**
* @param string $id
*/
#[\PHPUnit\Framework\Attributes\DataProvider('dataTestSplitUserRemoteError')]
#[DataProvider('dataTestSplitUserRemoteError')]
public function testSplitUserRemoteError($id): void {
$this->expectException(\Exception::class);

Expand Down Expand Up @@ -221,6 +222,148 @@ function ($appName, $key, $default) {
$this->assertTrue($result['exact']['remotes'][0]['value']['isTrustedServer']);
}

public static function dataLocalUserCloudIdVariants(): array {
return [
['someUserId@cloud.example.com', false],
['someUserId@cloud.example.com/', false],
['someUserId@cloud.example.com/index.php', false],
['someUserId@https://cloud.example.com', false],
['someUserId@http://cloud.example.com/', false],
['someUserId@Cloud.Example.com', false],
['SomeUserID@cloud.example.com', false],
['someUserId@example.com', true],
['someUserId@cloud.example.com/nextcloud', true],
['someUserId@HTTPS://CLOUD.example.com', false],
['someUserId@cloud.example.com:8443', true],
['someUserId@cloud.example.com:8443', false, 'someUserId@cloud.example.com:8443'],
['someUserId@cloud.example.com', true, 'someUserId@cloud.example.com:8443'],
['someUserId@Cloud.Example.com/nextcloud/', false, 'someUserId@cloud.example.com/nextcloud'],
['someUserId@cloud.example.com/Nextcloud', true, 'someUserId@cloud.example.com/nextcloud'],
];
}

#[DataProvider('dataLocalUserCloudIdVariants')]
public function testSearchLocalUserCloudIdVariants(string $search, bool $expectRemote, string $localCloudId = 'someUserId@cloud.example.com'): void {
$this->instantiatePlugin();

$localUser = $this->createMock(IUser::class);
$localUser->method('getUID')->willReturn('someUserId');
$localUser->method('getCloudId')->willReturn($localCloudId);
$this->userManager->method('get')
->willReturnCallback(fn (string $uid) => strtolower($uid) === 'someuserid' ? $localUser : null);

$this->contactsManager->method('search')->willReturn([]);

$this->plugin->search($search, 2, 0, $this->searchResult);
$result = $this->searchResult->asArray();

if ($expectRemote) {
$this->assertCount(1, $result['exact']['remotes']);
$this->assertSame($search, $result['exact']['remotes'][0]['value']['shareWith']);
} else {
$this->assertSame([], $result['exact']['remotes']);
}
}

public function testSearchContactWithLocalUserCloudIdVariant(): void {
$this->config->method('getAppValue')
->willReturnCallback(fn (string $app, string $key, $default) => $key === 'shareapi_allow_share_dialog_user_enumeration' ? 'yes' : $default);
$this->instantiatePlugin();

$localUser = $this->createMock(IUser::class);
$localUser->method('getUID')->willReturn('someUserId');
$localUser->method('getCloudId')->willReturn('someUserId@cloud.example.com');
$this->userManager->method('get')
->willReturnCallback(fn (string $uid) => strtolower($uid) === 'someuserid' ? $localUser : null);

$this->contactsManager->method('search')->willReturn([
[
'UID' => 'contact-id',
'FN' => 'Some User',
'EMAIL' => 'some.user@example.com',
'CLOUD' => ['SomeUserID@https://cloud.example.com/'],
],
]);

$this->plugin->search('Some', 2, 0, $this->searchResult);
$result = $this->searchResult->asArray();

$this->assertContains([
'label' => 'Some User',
'uuid' => 'contact-id',
'value' => [
'shareType' => IShare::TYPE_USER,
'shareWith' => 'someUserId',
],
'shareWithDisplayNameUnique' => 'some.user@example.com',
], $result['remotes']);
$this->assertSame([], $this->getRemoteShareTypeResults($result));
}

public function testSearchContactWithLocalUserCloudIdMatchingEmail(): void {
$this->config->method('getAppValue')
->willReturnCallback(fn (string $app, string $key, $default) => $key === 'shareapi_allow_share_dialog_user_enumeration' ? 'yes' : $default);
$this->instantiatePlugin();

$localUser = $this->createMock(IUser::class);
$localUser->method('getUID')->willReturn('someUserId');
$localUser->method('getCloudId')->willReturn('someUserId@cloud.example.com');
$this->userManager->method('get')
->willReturnCallback(fn (string $uid) => $uid === 'someUserId' ? $localUser : null);

$this->contactsManager->method('search')->willReturn([
[
'UID' => 'contact-id',
'FN' => 'Some User',
'EMAIL' => 'some.user@example.com',
'CLOUD' => ['someUserId@cloud.example.com'],
],
]);

$this->plugin->search('some.user@example.com', 2, 0, $this->searchResult);
$result = $this->searchResult->asArray();

$remoteShareWith = array_map(static fn (array $entry): string => $entry['value']['shareWith'], $this->getRemoteShareTypeResults($result));
$this->assertNotContains('someUserId@cloud.example.com', $remoteShareWith);
$this->assertFalse($this->searchResult->hasExactIdMatch(new SearchResultType('remotes')));
$this->assertCount(1, $result['remotes']);
$this->assertSame(IShare::TYPE_USER, $result['remotes'][0]['value']['shareType']);
}

public function testSearchContactWithCurrentUserCloudId(): void {
$this->config->method('getAppValue')
->willReturnCallback(fn (string $app, string $key, $default) => $key === 'shareapi_allow_share_dialog_user_enumeration' ? 'yes' : $default);
$this->instantiatePlugin();

$currentUser = $this->createMock(IUser::class);
$currentUser->method('getUID')->willReturn('admin');
$currentUser->method('getCloudId')->willReturn('admin@cloud.example.com');
$this->userManager->method('get')
->willReturnCallback(fn (string $uid) => $uid === 'admin' ? $currentUser : null);

$this->contactsManager->method('search')->willReturn([
[
'UID' => 'contact-id',
'FN' => 'Admin',
'EMAIL' => 'admin@example.com',
'CLOUD' => ['admin@cloud.example.com'],
],
]);

$this->plugin->search('Admin', 2, 0, $this->searchResult);
$result = $this->searchResult->asArray();

$this->assertSame([], $result['remotes']);
$this->assertSame([], $result['exact']['remotes']);
}

private function getRemoteShareTypeResults(array $result): array {
return array_values(array_filter(
array_merge($result['remotes'], $result['exact']['remotes']),
static fn (array $entry): bool => $entry['value']['shareType'] === IShare::TYPE_REMOTE,
));
}

public static function dataGetRemote() {
return [
['test', [], true, ['remotes' => [], 'exact' => ['remotes' => []]], false, true],
Expand Down
Loading