From 13acc220330c8c087350ae2cd12551d15d90307d Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Thu, 23 Jul 2026 14:43:34 +0200 Subject: [PATCH] fix(iMIP): Prevent mails from carrying an unrelated user's name Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: David Dreschner --- apps/dav/lib/CalDAV/Schedule/IMipPlugin.php | 57 +++- apps/dav/lib/Server.php | 2 + .../CalDAV/Schedule/IMipPluginCharsetTest.php | 3 + .../unit/CalDAV/Schedule/IMipPluginTest.php | 243 ++++++++++++++++++ 4 files changed, 299 insertions(+), 6 deletions(-) diff --git a/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php b/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php index 18d7559f1a702..44b35787c1387 100644 --- a/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php +++ b/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php @@ -11,9 +11,12 @@ use OCA\DAV\CalDAV\CalendarObject; use OCA\DAV\CalDAV\EventComparisonService; +use OCP\Accounts\IAccountManager; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\IAppConfig; +use OCP\IUser; +use OCP\IUserManager; use OCP\IUserSession; use OCP\Mail\IEmailValidator; use OCP\Mail\IMailer; @@ -66,6 +69,8 @@ public function __construct( private EventComparisonService $eventComparisonService, private IMailManager $mailManager, private IEmailValidator $emailValidator, + private IUserManager $userManager, + private IAccountManager $accountManager, ) { parent::__construct(''); } @@ -184,21 +189,18 @@ public function schedule(Message $iTipMessage) { } $this->imipService->setL10nFromAttendee($attendee); + $sender = substr($iTipMessage->sender, 7); + // Build the sender name. // Due to a bug in sabre, the senderName property for an iTIP message can actually also be a VObject Property - // If the iTIP message senderName is null or empty use the user session name as the senderName if (($iTipMessage->senderName instanceof Parameter) && !empty(trim($iTipMessage->senderName->getValue()))) { $senderName = trim($iTipMessage->senderName->getValue()); } elseif (is_string($iTipMessage->senderName) && !empty(trim($iTipMessage->senderName))) { $senderName = trim($iTipMessage->senderName); - } elseif ($this->userSession->getUser() !== null) { - $senderName = trim($this->userSession->getUser()->getDisplayName()); } else { - $senderName = ''; + $senderName = $this->getSenderNameFor($sender); } - $sender = substr($iTipMessage->sender, 7); - $replyingAttendee = null; switch (strtolower($iTipMessage->method)) { case self::METHOD_REPLY: @@ -338,6 +340,49 @@ public function schedule(Message $iTipMessage) { } } + /** + * Resolves a display name for a sender address when the iTip message + * carries no CN parameter. + * + * Messages are regularly brokered on behalf of somebody else, so the + * session user's name is only used when the address is one of theirs. + * Otherwise the address must map unambiguously to a single local user, + * matching how login by email treats ambiguous addresses. + */ + private function getSenderNameFor(string $sender): ?string { + $user = $this->userSession->getUser(); + if ($user !== null && $this->isAddressOfUser($sender, $user)) { + return trim($user->getDisplayName()); + } + + $candidates = $this->userManager->getByEmail($sender); + if (count($candidates) === 1) { + return trim($candidates[0]->getDisplayName()); + } + + return null; + } + + /** + * Whether the address is the user's system email address or one of the + * profile email addresses advertised in their calendar-user-address-set. + */ + private function isAddressOfUser(string $address, IUser $user): bool { + if (strcasecmp((string)$user->getEMailAddress(), $address) === 0) { + return true; + } + + $emailCollection = $this->accountManager->getAccount($user) + ->getPropertyCollection(IAccountManager::COLLECTION_EMAIL); + foreach ($emailCollection->getProperties() as $property) { + if (strcasecmp($property->getValue(), $address) === 0) { + return true; + } + } + + return false; + } + /** * @return ?VCalendar */ diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index fdaa35e4a4d16..f734e0cc3972b 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -360,6 +360,8 @@ public function __construct( \OCP\Server::get(EventComparisonService::class), \OCP\Server::get(\OCP\Mail\Provider\IManager::class), \OCP\Server::get(IEmailValidator::class), + \OCP\Server::get(\OCP\IUserManager::class), + \OCP\Server::get(IAccountManager::class), )); } $this->server->addPlugin(new \OCA\DAV\CalDAV\Search\SearchPlugin()); diff --git a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginCharsetTest.php b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginCharsetTest.php index ec6a4567e9c92..92c1e0d91f33f 100644 --- a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginCharsetTest.php +++ b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginCharsetTest.php @@ -13,6 +13,7 @@ use OCA\DAV\CalDAV\EventComparisonService; use OCA\DAV\CalDAV\Schedule\IMipPlugin; use OCA\DAV\CalDAV\Schedule\IMipService; +use OCP\Accounts\IAccountManager; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Config\IUserConfig; use OCP\Defaults; @@ -132,6 +133,8 @@ protected function setUp(): void { $this->eventComparisonService, $this->mailManager, $this->getEmailValidatorWithStrictEmailCheck(), + $this->userManager, + $this->createMock(IAccountManager::class), ); // ITipMessage diff --git a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php index 048dcd13b67e3..297bdf995825f 100644 --- a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php +++ b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php @@ -12,15 +12,21 @@ use OCA\DAV\CalDAV\EventComparisonService; use OCA\DAV\CalDAV\Schedule\IMipPlugin; use OCA\DAV\CalDAV\Schedule\IMipService; +use OCP\Accounts\IAccount; +use OCP\Accounts\IAccountManager; +use OCP\Accounts\IAccountProperty; +use OCP\Accounts\IAccountPropertyCollection; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\IAppConfig; use OCP\IUser; +use OCP\IUserManager; use OCP\IUserSession; use OCP\Mail\IAttachment; use OCP\Mail\IEMailTemplate; use OCP\Mail\IMailer; use OCP\Mail\IMessage; +use OCP\Mail\Provider\Address; use OCP\Mail\Provider\IManager as IMailManager; use OCP\Mail\Provider\IMessage as IMailMessageNew; use OCP\Mail\Provider\IMessageSend as IMailMessageSend; @@ -58,6 +64,8 @@ class IMipPluginTest extends TestCase { private IMailManager&MockObject $mailManager; private IMailServiceMock&MockObject $mailService; private IMailMessageNew&MockObject $mailMessageNew; + private IUserManager&MockObject $userManager; + private IAccountManager&MockObject $accountManager; protected function setUp(): void { $this->mailMessage = $this->createMock(IMessage::class); @@ -101,6 +109,9 @@ protected function setUp(): void { $this->mailMessageNew = $this->createMock(IMailMessageNew::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->accountManager = $this->createMock(IAccountManager::class); + $this->plugin = new IMipPlugin( $this->config, $this->mailer, @@ -112,6 +123,8 @@ protected function setUp(): void { $this->eventComparisonService, $this->mailManager, $this->getEmailValidatorWithStrictEmailCheck(), + $this->userManager, + $this->accountManager, ); } @@ -213,6 +226,9 @@ public function testParsingSingle(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -505,6 +521,9 @@ public function testParsingRecurrence(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -632,6 +651,9 @@ public function testFailedDelivery(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -850,6 +872,9 @@ public function testMailProviderDisabled(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -953,6 +978,9 @@ public function testNoOldEvent(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -1052,6 +1080,9 @@ public function testNoButtons(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -1203,6 +1234,9 @@ public function testExternalAttendeesDisabledForSystemUser(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -1237,4 +1271,213 @@ public function testExternalAttendeesDisabledForSystemUser(): void { $this->plugin->schedule($message); $this->assertEquals('1.1', $message->getScheduleStatus()); } + + /** + * Runs schedule() for an organizer sourced REQUEST whose iTip message + * carries no sender name and captures the resulting From and Reply-To + * headers, sent either via the system or the user's mail account. + * + * @return array{from: ?array, replyTo: ?array} + */ + private function scheduleWithoutSenderName(string $organizer, string $recipient, bool $viaMailProvider = false): array { + $vCalendar = new VCalendar(); + $vEvent = new VEvent($vCalendar, 'VEVENT', [ + 'UID' => 'uid-1234', + 'SEQUENCE' => 1, + 'SUMMARY' => 'Meeting', + 'DTSTART' => new \DateTime('2017-01-01 00:00:00'), + ]); + $vEvent->add('ORGANIZER', 'mailto:' . $organizer); + $vEvent->add('ATTENDEE', 'mailto:' . $recipient, ['RSVP' => 'TRUE', 'CN' => 'Frodo']); + + $message = new Message(); + $message->method = 'REQUEST'; + $message->message = $vCalendar; + $message->sender = 'mailto:' . $organizer; + $message->senderName = null; + $message->recipient = 'mailto:' . $recipient; + + $capturedFrom = null; + $capturedReplyTo = null; + $mailMessage = $this->createMock(IMessage::class); + $mailMessage->method('setTo')->willReturn($mailMessage); + $mailMessage->method('setFrom') + ->willReturnCallback(function (array $from) use (&$capturedFrom, $mailMessage) { + $capturedFrom = $from; + return $mailMessage; + }); + $mailMessage->method('setReplyTo') + ->willReturnCallback(function (array $replyTo) use (&$capturedReplyTo, $mailMessage) { + $capturedReplyTo = $replyTo; + return $mailMessage; + }); + + $mailer = $this->createMock(IMailer::class); + $mailer->method('createMessage')->willReturn($mailMessage); + $mailer->method('createEMailTemplate')->willReturn($this->emailTemplate); + $mailer->method('send')->willReturn([]); + + if ($viaMailProvider) { + $this->mailMessageNew->method('setFrom') + ->willReturnCallback(function (Address $from) use (&$capturedFrom) { + $capturedFrom = [$from->getAddress() => (string)$from->getLabel()]; + return $this->mailMessageNew; + }); + $this->mailService->method('initiateMessage')->willReturn($this->mailMessageNew); + $this->mailService->expects(self::once()) + ->method('sendMessage') + ->with($this->mailMessageNew); + $this->mailManager->method('findServiceByAddress')->willReturn($this->mailService); + } + + $this->service->method('getLastOccurrence')->willReturn(1496912700); + $this->service->method('getCurrentAttendee')->willReturn($vEvent->select('ATTENDEE')[0]); + $this->service->method('isRoomOrResource')->willReturn(false); + $this->service->method('isCircle')->willReturn(false); + $this->service->method('getAttendeeRsvpOrReqForParticipant')->willReturn(false); + $this->service->method('buildBodyData')->willReturn([ + 'meeting_title' => 'Meeting', + 'invitee_name' => '', + 'attendee_name' => $recipient, + ]); + // Mirrors the real IMipService::getFrom() so assertions read like the + // actual mail header. + $this->service->method('getFrom') + ->willReturnCallback(static fn (?string $senderName, string $default): string + => ($senderName === null || $senderName === '') ? $default : $senderName . ' via ' . $default); + + $this->config->method('getValueBool')->willReturnMap([ + ['dav', 'caldav_external_attendees_disabled', false, false], + ['core', 'mail_providers_enabled', true, $viaMailProvider], + ]); + $this->eventComparisonService->method('findModified') + ->willReturn(['old' => [], 'new' => [$vEvent]]); + + $plugin = new IMipPlugin( + $this->config, + $mailer, + $this->logger, + $this->timeFactory, + $this->defaults, + $this->userSession, + $this->service, + $this->eventComparisonService, + $this->mailManager, + $this->getEmailValidatorWithStrictEmailCheck(), + $this->userManager, + $this->accountManager, + ); + $plugin->schedule($message); + self::assertSame('1.1', $message->getScheduleStatus()); + + return ['from' => $capturedFrom, 'replyTo' => $capturedReplyTo]; + } + + /** + * Messages are regularly brokered on behalf of somebody else, so headers + * must not fall back to the session user's name when the sender address + * is not theirs. + */ + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameIsNotTakenFromAnUnrelatedSessionUser(bool $viaMailProvider): void { + $this->user->method('getUID')->willReturn('bilbo'); + $this->user->method('getDisplayName')->willReturn('Bilbo Baggins'); + $this->user->method('getEMailAddress')->willReturn('bilbo@hobb.it'); + $this->userManager->method('getByEmail')->with('a@example.com')->willReturn([]); + + $result = $this->scheduleWithoutSenderName('a@example.com', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['a@example.com'], $result['replyTo']); + } + } + + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameFallsBackToSessionUserWhenTheyAreTheSender(bool $viaMailProvider): void { + $this->user->method('getUID')->willReturn('gandalf'); + $this->user->method('getDisplayName')->willReturn('Mr. Wizard'); + $this->user->method('getEMailAddress')->willReturn('gandalf@wiz.ard'); + + $result = $this->scheduleWithoutSenderName('gandalf@wiz.ard', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Mr. Wizard via Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['gandalf@wiz.ard' => 'Mr. Wizard'], $result['replyTo']); + } + } + + /** + * The session user must also be recognized as the sender when sending + * under one of their profile email aliases. + */ + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameUsesSessionUserForTheirAliasAddress(bool $viaMailProvider): void { + $this->user->method('getUID')->willReturn('carl'); + $this->user->method('getDisplayName')->willReturn('Carl Session'); + $this->user->method('getEMailAddress')->willReturn('carl@example.com'); + + $aliasProperty = $this->createMock(IAccountProperty::class); + $aliasProperty->method('getValue')->willReturn('Shared@Corp.example'); + $emailCollection = $this->createMock(IAccountPropertyCollection::class); + $emailCollection->method('getProperties')->willReturn([$aliasProperty]); + $account = $this->createMock(IAccount::class); + $account->method('getPropertyCollection') + ->with(IAccountManager::COLLECTION_EMAIL) + ->willReturn($emailCollection); + $this->accountManager->method('getAccount')->with($this->user)->willReturn($account); + $this->userManager->expects(self::never())->method('getByEmail'); + + $result = $this->scheduleWithoutSenderName('shared@corp.example', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Carl Session via Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['shared@corp.example' => 'Carl Session'], $result['replyTo']); + } + } + + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameIsResolvedFromTheAddressOwner(bool $viaMailProvider): void { + $this->user->method('getUID')->willReturn('bilbo'); + $this->user->method('getDisplayName')->willReturn('Bilbo Baggins'); + $this->user->method('getEMailAddress')->willReturn('bilbo@hobb.it'); + + $organizerUser = $this->createMock(IUser::class); + $organizerUser->method('getDisplayName')->willReturn('Anna Organizer'); + $this->userManager->method('getByEmail')->with('a@example.com')->willReturn([$organizerUser]); + + $result = $this->scheduleWithoutSenderName('a@example.com', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Anna Organizer via Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['a@example.com' => 'Anna Organizer'], $result['replyTo']); + } + } + + /** + * Emails are not unique across users - login by email declines ambiguous + * addresses the same way (\OC\User\Session::logClientIn()). + */ + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameStaysNeutralForAmbiguousAddresses(bool $viaMailProvider): void { + $this->user->method('getEMailAddress')->willReturn('bilbo@hobb.it'); + + $owner1 = $this->createMock(IUser::class); + $owner2 = $this->createMock(IUser::class); + $this->userManager->method('getByEmail')->with('office@corp.example')->willReturn([$owner1, $owner2]); + + $result = $this->scheduleWithoutSenderName('office@corp.example', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['office@corp.example'], $result['replyTo']); + } + } + + public static function transportProvider(): array { + return [ + 'system email account' => [false], + 'user email account' => [true], + ]; + } }