From 00b5dd59a6727963cf69c0028a545843379526cd Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Sat, 2 May 2026 19:46:58 +0200 Subject: [PATCH] fix(security): escape link URLs in activity email templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The activity-digest and queued-mail templates interpolate the `link` attribute of rich-subject parameters and the home URL directly into the `href` of an `` tag without HTML-escaping. Activity providers typically supply internal Nextcloud URLs, but defensive escaping closes a potential `"`-breakout XSS vector should a provider ever expose user-influenced link values. Wrap the URLs in `htmlspecialchars(..., ENT_QUOTES, 'UTF-8')` in: - DigestSender.php — "and N more" link, rich-subject link parameter - MailQueueHandler.php — home link, rich-subject link parameter Translation-string templates (`$l->t('......', ...)`) are left as-is: their URLs come exclusively from `linkToRouteAbsolute()`, and escaping inside the gettext source string would force translators to handle escaping for every locale. Signed-off-by: Frank Karlitschek --- lib/DigestSender.php | 4 +- lib/MailQueueHandler.php | 4 +- tests/DigestSenderTest.php | 83 ++++++++++++++++++++++++++++++++++ tests/MailQueueHandlerTest.php | 35 ++++++++++++++ 4 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 tests/DigestSenderTest.php diff --git a/lib/DigestSender.php b/lib/DigestSender.php index 0dfe4d380..f2bc54977 100644 --- a/lib/DigestSender.php +++ b/lib/DigestSender.php @@ -188,7 +188,7 @@ public function sendDigestForUser(IUser $user, int $now, string $timezone, strin $andMoreText = $l10n->n('and %n more…', 'and %n more…', $skippedCount); $url = $this->urlGenerator->linkToRouteAbsolute('activity.Activities.showList', [ 'filter' => 'all' ]); $template->addBodyListItem( - '' . htmlspecialchars($andMoreText) . '', + '' . htmlspecialchars($andMoreText) . '', plainText: $andMoreText, ); } @@ -235,7 +235,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 46be003e4..ae6087175 100644 --- a/lib/MailQueueHandler.php +++ b/lib/MailQueueHandler.php @@ -331,7 +331,7 @@ function ($event) use ($timezone, $l) { $template->addHeader(); $template->addHeading($l->t('Hello %s', [$user->getDisplayName()]), $l->t('Hello %s,', [$user->getDisplayName()])); - $homeLink = '' . htmlspecialchars($this->getSenderData('name')) . ''; + $homeLink = '' . htmlspecialchars($this->getSenderData('name')) . ''; $template->addBodyText( $l->t('There was some activity at %s', [$homeLink]), $l->t('There was some activity at %s', [$this->urlGenerator->getAbsoluteURL('/')]) @@ -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..c87f33272 --- /dev/null +++ b/tests/DigestSenderTest.php @@ -0,0 +1,83 @@ +digestSender = new DigestSender( + $this->createMock(IConfig::class), + $this->createMock(Data::class), + $this->createMock(UserSettings::class), + $this->createMock(GroupHelper::class), + $this->createMock(IMailer::class), + $this->createMock(IManager::class), + $this->createMock(IUserManager::class), + $this->createMock(IURLGenerator::class), + $this->createMock(Defaults::class), + $this->createMock(IFactory::class), + $this->createMock(IDateTimeFormatter::class), + $this->createMock(LoggerInterface::class), + ); + } + + public static function provideGetHTMLSubjectData(): array { + return [ + 'no rich subject escapes parsed subject' => [ + '', 'Hello ', [], + 'Hello <World>', + ], + 'linked parameter renders anchor' => [ + 'File {file} was shared', '', + ['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg', 'link' => 'https://cloud.example.com/f/123']], + 'File photo.jpg was shared', + ], + 'double-quote in link is escaped' => [ + 'File {file} was shared', '', + ['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg', 'link' => 'https://cloud.example.com/f/123"onmouseover="alert(1)']], + 'File photo.jpg was shared', + ], + 'parameter without link uses strong tag' => [ + 'File {file} was shared', '', + ['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg']], + 'File photo.jpg was shared', + ], + ]; + } + + #[DataProvider('provideGetHTMLSubjectData')] + public function testGetHTMLSubject(string $richSubject, string $parsedSubject, array $richParams, string $expected): void { + $event = $this->createMock(IEvent::class); + $event->method('getRichSubject')->willReturn($richSubject); + $event->method('getParsedSubject')->willReturn($parsedSubject); + $event->method('getRichSubjectParameters')->willReturn($richParams); + + $result = self::invokePrivate($this->digestSender, 'getHTMLSubject', [$event]); + $this->assertSame($expected, $result); + } +} diff --git a/tests/MailQueueHandlerTest.php b/tests/MailQueueHandlerTest.php index 91a658138..61c08537c 100644 --- a/tests/MailQueueHandlerTest.php +++ b/tests/MailQueueHandlerTest.php @@ -375,6 +375,41 @@ public function testSendEmailsDeletesQueueOnSendReturnFalse(): void { * @param int $maxTime * @param string $explain */ + public static function provideGetHTMLSubjectData(): array { + return [ + 'no rich subject escapes parsed subject' => [ + '', 'Hello ', [], + 'Hello <World>', + ], + 'linked parameter renders anchor' => [ + 'File {file} was shared', '', + ['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg', 'link' => 'https://cloud.example.com/f/123']], + 'File photo.jpg was shared', + ], + 'double-quote in link is escaped' => [ + 'File {file} was shared', '', + ['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg', 'link' => 'https://cloud.example.com/f/123"onmouseover="alert(1)']], + 'File photo.jpg was shared', + ], + 'parameter without link uses strong tag' => [ + 'File {file} was shared', '', + ['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg']], + 'File photo.jpg was shared', + ], + ]; + } + + #[DataProvider('provideGetHTMLSubjectData')] + public function testGetHTMLSubject(string $richSubject, string $parsedSubject, array $richParams, string $expected): void { + $event = $this->createMock(IEvent::class); + $event->method('getRichSubject')->willReturn($richSubject); + $event->method('getParsedSubject')->willReturn($parsedSubject); + $event->method('getRichSubjectParameters')->willReturn($richParams); + + $result = self::invokePrivate($this->mailQueueHandler, 'getHTMLSubject', [$event]); + $this->assertSame($expected, $result); + } + protected function assertRemainingMailEntries(array $users, int $maxTime, string $explain): void { foreach ($users as $user) { [$data,] = self::invokePrivate($this->mailQueueHandler, 'getItemsForUser', [$user, $maxTime]);