From 84c41aa873cc657f6cd681260762b9e963153222 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:09:20 +0200 Subject: [PATCH] fix(mail): escape rich object links and skip unsendable digest users The `link` of a rich object was interpolated straight into the href of activity and digest mails while the link text beside it was escaped, so a parameter carrying a quote closed the attribute and injected markup into the mail body. Two digest problems alongside it: - `new DateTimeZone()` sat outside the per-user try/catch, so a single unparseable `core/timezone` preference threw out of the loop and nobody later in the batch received a digest. The verdict is now memoised, so a shared bad value is reported once instead of once per user. - Users without an email address were never skipped. The whole digest was built and handed to the mailer only for it to throw, once per user, on every run. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/DigestSender.php | 27 +++-- lib/MailQueueHandler.php | 2 +- tests/DigestSenderTest.php | 184 +++++++++++++++++++++++++++++++++ tests/MailQueueHandlerTest.php | 23 +++++ 4 files changed, 228 insertions(+), 8 deletions(-) create mode 100644 tests/DigestSenderTest.php diff --git a/lib/DigestSender.php b/lib/DigestSender.php index 35b5fc9e0..dc3b875e4 100644 --- a/lib/DigestSender.php +++ b/lib/DigestSender.php @@ -55,13 +55,22 @@ public function sendDigests(int $now): void { $timezone = (!empty($userTimezones[$user])) ? $userTimezones[$user] : $defaultTimeZone; // Check if the user's timezone is after 6am already - if (!isset($timezoneDigestDay[$timezone])) { - $timezoneDate = new \DateTime('now', new \DateTimeZone($timezone)); - if ($timezoneDate->format('H') < 6) { - // Still before 6am, so dont send yet. - $timezoneDate->sub(new \DateInterval('P1D')); + if (!array_key_exists($timezone, $timezoneDigestDay)) { + try { + $timezoneDate = new \DateTime('now', new \DateTimeZone($timezone)); + if ($timezoneDate->format('H') < 6) { + // Still before 6am, so dont send yet. + $timezoneDate->sub(new \DateInterval('P1D')); + } + $timezoneDigestDay[$timezone] = $timezoneDate->format('Y.m.d'); + } catch (\Exception $e) { + // A single broken user timezone must not abort the whole run + $this->logger->warning('Invalid timezone "' . $timezone . '", skipping digest', ['exception' => $e]); + $timezoneDigestDay[$timezone] = null; } - $timezoneDigestDay[$timezone] = $timezoneDate->format('Y.m.d'); + } + if ($timezoneDigestDay[$timezone] === null) { + continue; } $userDigestDate = $digestDate[$user] ?? ''; @@ -75,6 +84,10 @@ public function sendDigests(int $now): void { $this->logger->info("User $user could not be found when sending user digest emails"); continue; } + if (empty($userObject->getEMailAddress())) { + $this->updateLastSentForUser($userObject, $now); + continue; + } if (!$userObject->isEnabled()) { // User is disabled so do not send the email but update last sent since after enabling avoid flooding $this->updateLastSentForUser($userObject, $now); @@ -236,7 +249,7 @@ protected function getHTMLSubject(IEvent $event): string { } if (isset($parameter['link'])) { - $replacements[] = '' . htmlspecialchars($replacement) . ''; + $replacements[] = '' . htmlspecialchars($replacement) . ''; } else { $replacements[] = '' . htmlspecialchars($replacement) . ''; } diff --git a/lib/MailQueueHandler.php b/lib/MailQueueHandler.php index eaaa4a59a..1f6cc28a5 100644 --- a/lib/MailQueueHandler.php +++ b/lib/MailQueueHandler.php @@ -394,7 +394,7 @@ protected function getHTMLSubject(IEvent $event): string { } if (isset($parameter['link'])) { - $replacements[] = '' . htmlspecialchars($replacement) . ''; + $replacements[] = '' . htmlspecialchars($replacement) . ''; } else { $replacements[] = '' . htmlspecialchars($replacement) . ''; } diff --git a/tests/DigestSenderTest.php b/tests/DigestSenderTest.php new file mode 100644 index 000000000..670946b74 --- /dev/null +++ b/tests/DigestSenderTest.php @@ -0,0 +1,184 @@ +config = $this->createMock(IConfig::class); + $this->data = $this->createMock(Data::class); + $this->mailer = $this->createMock(IMailer::class); + $this->activityManager = $this->createMock(IManager::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $l10nFactory = $this->createMock(IFactory::class); + $l10nFactory->method('get') + ->willReturn($this->createMock(IL10N::class)); + + $this->digestSender = new DigestSender( + $this->config, + $this->data, + $this->createMock(UserSettings::class), + $this->createMock(GroupHelper::class), + $this->mailer, + $this->activityManager, + $this->userManager, + $this->createMock(IURLGenerator::class), + $this->createMock(Defaults::class), + $l10nFactory, + $this->createMock(IDateTimeFormatter::class), + $this->logger, + ); + } + + protected function expectDigestUsers(array $users, array $timezones): void { + $this->config->method('getUsersForUserValue') + ->willReturn($users); + $this->config->method('getUserValueForUsers') + ->willReturnCallback(static function (string $app, string $key) use ($timezones) { + return ($app === 'core' && $key === 'timezone') ? $timezones : []; + }); + } + + protected function createUser(string $uid, string $email): IUser&MockObject { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + $user->method('getEMailAddress')->willReturn($email); + $user->method('isEnabled')->willReturn(true); + return $user; + } + + public function testInvalidTimezoneDoesNotAbortTheRun(): void { + $this->expectDigestUsers( + ['brokenUser', 'goodUser'], + ['brokenUser' => 'Not/AZone', 'goodUser' => 'UTC'], + ); + + $this->userManager->expects($this->once()) + ->method('get') + ->with('goodUser') + ->willReturn($this->createUser('goodUser', '')); + $this->logger->expects($this->once()) + ->method('warning'); + + $this->digestSender->sendDigests(1700000000); + } + + public function testInvalidTimezoneIsOnlyReportedOnce(): void { + $this->expectDigestUsers( + ['userA', 'userB', 'userC'], + ['userA' => 'Not/AZone', 'userB' => 'Not/AZone', 'userC' => 'Not/AZone'], + ); + + $this->userManager->expects($this->never()) + ->method('get'); + $this->logger->expects($this->once()) + ->method('warning'); + + $this->digestSender->sendDigests(1700000000); + } + + public function testUserWithoutEmailIsSkippedBeforeBuildingTheDigest(): void { + $this->expectDigestUsers(['noMailUser'], ['noMailUser' => 'UTC']); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('noMailUser'); + $user->method('getEMailAddress')->willReturn(''); + // Reached only if the missing address is not caught first + $user->expects($this->never()) + ->method('isEnabled'); + + $this->userManager->method('get') + ->willReturn($user); + $this->data->method('getActivitySince') + ->willReturn(['count' => 0, 'max' => 0]); + + $this->mailer->expects($this->never()) + ->method('createEMailTemplate'); + $this->mailer->expects($this->never()) + ->method('send'); + // The marker still moves so enabling an address later does not flood them + $this->config->expects($this->once()) + ->method('setUserValue') + ->with('noMailUser', 'activity', 'activity_digest_last_send', $this->anything()); + + $this->digestSender->sendDigests(1700000000); + } + + public function testCurrentUserIsResetWhenThereIsNothingToSend(): void { + $user = $this->createUser('someUser', 'user@example.com'); + + $this->config->method('getUserValue') + ->willReturn('5'); + $this->data->method('getActivitySince') + ->willReturn(['count' => 0, 'max' => 5]); + + $resetUsers = []; + $this->activityManager->method('setCurrentUserId') + ->willReturnCallback(static function (?string $uid) use (&$resetUsers): void { + $resetUsers[] = $uid; + }); + + $this->digestSender->sendDigestForUser($user, 1700000000, 'UTC', 'en'); + + // The identity must not survive the early return + $this->assertSame(['someUser', null], $resetUsers); + } + + public function testGetHTMLSubjectEscapesParameters(): void { + $this->assertSame( + 'Shared <b>secret</b>.txt', + $this->formatSubject(['file' => ['type' => 'file', 'path' => 'secret.txt']]), + ); + $this->assertSame( + 'Shared secret.txt', + $this->formatSubject(['file' => [ + 'type' => 'file', + 'path' => 'secret.txt', + 'link' => 'https://example.com/">', + ]]), + ); + } + + protected function formatSubject(array $parameters): string { + $event = $this->createMock(IEvent::class); + $event->method('getRichSubject')->willReturn('Shared {file}'); + $event->method('getRichSubjectParameters')->willReturn($parameters); + + return self::invokePrivate($this->digestSender, 'getHTMLSubject', [$event]); + } +} diff --git a/tests/MailQueueHandlerTest.php b/tests/MailQueueHandlerTest.php index c1f45f684..293efe71b 100644 --- a/tests/MailQueueHandlerTest.php +++ b/tests/MailQueueHandlerTest.php @@ -402,6 +402,29 @@ public function testSendEmailsSkipsWhenAdminEmailDisabled(): void { } } + public function testGetHTMLSubjectEscapesParameters(): void { + $this->assertSame( + 'Shared <b>secret</b>.txt', + $this->formatSubject(['file' => ['type' => 'file', 'path' => 'secret.txt']]), + ); + $this->assertSame( + 'Shared secret.txt', + $this->formatSubject(['file' => [ + 'type' => 'file', + 'path' => 'secret.txt', + 'link' => 'https://example.com/">', + ]]), + ); + } + + protected function formatSubject(array $parameters): string { + $event = $this->createMock(IEvent::class); + $event->method('getRichSubject')->willReturn('Shared {file}'); + $event->method('getRichSubjectParameters')->willReturn($parameters); + + return self::invokePrivate($this->mailQueueHandler, 'getHTMLSubject', [$event]); + } + public function testGetMailMaxItemsReturnsCapWhenValueExceedsCap(): void { $this->appConfig->method('getValueInt') ->with('activity', 'mail_max_items', $this->mailQueueHandler::MAIL_MAX_ITEMS_DEFAULT)