Skip to content

Commit 37b99bc

Browse files
karlitschekmiaulalala
karlitschek
authored and
miaulalala
committed
fix(security): escape link URLs in activity email templates
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 `<a>` 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('...<a href="%s">...', ...)`) 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 <frank@nextcloud.com>
1 parent 434b071 commit 37b99bc

4 files changed

Lines changed: 123 additions & 4 deletions

File tree

‎lib/DigestSender.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ public function sendDigestForUser(IUser $user, int $now, string $timezone, strin
188188
$andMoreText = $l10n->n('and %n more…', 'and %n more…', $skippedCount);
189189
$url = $this->urlGenerator->linkToRouteAbsolute('activity.Activities.showList', [ 'filter' => 'all' ]);
190190
$template->addBodyListItem(
191-
'<a href="' . $url . '">' . htmlspecialchars($andMoreText) . '</a>',
191+
'<a href="' . htmlspecialchars($url) . '">' . htmlspecialchars($andMoreText) . '</a>',
192192
plainText: $andMoreText,
193193
);
194194
}
@@ -235,7 +235,7 @@ protected function getHTMLSubject(IEvent $event): string {
235235
}
236236

237237
if (isset($parameter['link'])) {
238-
$replacements[] = '<a href="' . $parameter['link'] . '">' . htmlspecialchars($replacement) . '</a>';
238+
$replacements[] = '<a href="' . htmlspecialchars((string)$parameter['link']) . '">' . htmlspecialchars($replacement) . '</a>';
239239
} else {
240240
$replacements[] = '<strong>' . htmlspecialchars($replacement) . '</strong>';
241241
}

‎lib/MailQueueHandler.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ function ($event) use ($timezone, $l) {
331331
$template->addHeader();
332332
$template->addHeading($l->t('Hello %s', [$user->getDisplayName()]), $l->t('Hello %s,', [$user->getDisplayName()]));
333333

334-
$homeLink = '<a href="' . $this->urlGenerator->getAbsoluteURL('/') . '">' . htmlspecialchars($this->getSenderData('name')) . '</a>';
334+
$homeLink = '<a href="' . htmlspecialchars($this->urlGenerator->getAbsoluteURL('/')) . '">' . htmlspecialchars($this->getSenderData('name')) . '</a>';
335335
$template->addBodyText(
336336
$l->t('There was some activity at %s', [$homeLink]),
337337
$l->t('There was some activity at %s', [$this->urlGenerator->getAbsoluteURL('/')])
@@ -394,7 +394,7 @@ protected function getHTMLSubject(IEvent $event): string {
394394
}
395395

396396
if (isset($parameter['link'])) {
397-
$replacements[] = '<a href="' . $parameter['link'] . '">' . htmlspecialchars($replacement) . '</a>';
397+
$replacements[] = '<a href="' . htmlspecialchars((string)$parameter['link']) . '">' . htmlspecialchars($replacement) . '</a>';
398398
} else {
399399
$replacements[] = '<strong>' . htmlspecialchars($replacement) . '</strong>';
400400
}

‎tests/DigestSenderTest.php‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
9+
namespace OCA\Activity\Tests;
10+
11+
use OCA\Activity\Data;
12+
use OCA\Activity\DigestSender;
13+
use OCA\Activity\GroupHelper;
14+
use OCA\Activity\UserSettings;
15+
use OCP\Activity\IEvent;
16+
use OCP\Activity\IManager;
17+
use OCP\Defaults;
18+
use OCP\IConfig;
19+
use OCP\IDateTimeFormatter;
20+
use OCP\IURLGenerator;
21+
use OCP\IUserManager;
22+
use OCP\L10N\IFactory;
23+
use OCP\Mail\IMailer;
24+
use PHPUnit\Framework\Attributes\DataProvider;
25+
use PHPUnit\Framework\MockObject\MockObject;
26+
use Psr\Log\LoggerInterface;
27+
28+
class DigestSenderTest extends TestCase {
29+
private DigestSender $digestSender;
30+
31+
protected function setUp(): void {
32+
parent::setUp();
33+
34+
$this->digestSender = new DigestSender(
35+
$this->createMock(IConfig::class),
36+
$this->createMock(Data::class),
37+
$this->createMock(UserSettings::class),
38+
$this->createMock(GroupHelper::class),
39+
$this->createMock(IMailer::class),
40+
$this->createMock(IManager::class),
41+
$this->createMock(IUserManager::class),
42+
$this->createMock(IURLGenerator::class),
43+
$this->createMock(Defaults::class),
44+
$this->createMock(IFactory::class),
45+
$this->createMock(IDateTimeFormatter::class),
46+
$this->createMock(LoggerInterface::class),
47+
);
48+
}
49+
50+
public static function provideGetHTMLSubjectData(): array {
51+
return [
52+
'no rich subject escapes parsed subject' => [
53+
'', 'Hello <World>', [],
54+
'Hello &lt;World&gt;',
55+
],
56+
'linked parameter renders anchor' => [
57+
'File {file} was shared', '',
58+
['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg', 'link' => 'https://cloud.example.com/f/123']],
59+
'File <a href="https://cloud.example.com/f/123">photo.jpg</a> was shared',
60+
],
61+
'double-quote in link is escaped' => [
62+
'File {file} was shared', '',
63+
['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg', 'link' => 'https://cloud.example.com/f/123"onmouseover="alert(1)']],
64+
'File <a href="https://cloud.example.com/f/123&quot;onmouseover=&quot;alert(1)">photo.jpg</a> was shared',
65+
],
66+
'parameter without link uses strong tag' => [
67+
'File {file} was shared', '',
68+
['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg']],
69+
'File <strong>photo.jpg</strong> was shared',
70+
],
71+
];
72+
}
73+
74+
#[DataProvider('provideGetHTMLSubjectData')]
75+
public function testGetHTMLSubject(string $richSubject, string $parsedSubject, array $richParams, string $expected): void {
76+
$event = $this->createMock(IEvent::class);
77+
$event->method('getRichSubject')->willReturn($richSubject);
78+
$event->method('getParsedSubject')->willReturn($parsedSubject);
79+
$event->method('getRichSubjectParameters')->willReturn($richParams);
80+
81+
$result = self::invokePrivate($this->digestSender, 'getHTMLSubject', [$event]);
82+
$this->assertSame($expected, $result);
83+
}
84+
}

‎tests/MailQueueHandlerTest.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,41 @@ public function testSendEmailsDeletesQueueOnSendReturnFalse(): void {
375375
* @param int $maxTime
376376
* @param string $explain
377377
*/
378+
public static function provideGetHTMLSubjectData(): array {
379+
return [
380+
'no rich subject escapes parsed subject' => [
381+
'', 'Hello <World>', [],
382+
'Hello &lt;World&gt;',
383+
],
384+
'linked parameter renders anchor' => [
385+
'File {file} was shared', '',
386+
['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg', 'link' => 'https://cloud.example.com/f/123']],
387+
'File <a href="https://cloud.example.com/f/123">photo.jpg</a> was shared',
388+
],
389+
'double-quote in link is escaped' => [
390+
'File {file} was shared', '',
391+
['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg', 'link' => 'https://cloud.example.com/f/123"onmouseover="alert(1)']],
392+
'File <a href="https://cloud.example.com/f/123&quot;onmouseover=&quot;alert(1)">photo.jpg</a> was shared',
393+
],
394+
'parameter without link uses strong tag' => [
395+
'File {file} was shared', '',
396+
['file' => ['type' => 'file', 'path' => 'photo.jpg', 'name' => 'photo.jpg']],
397+
'File <strong>photo.jpg</strong> was shared',
398+
],
399+
];
400+
}
401+
402+
#[DataProvider('provideGetHTMLSubjectData')]
403+
public function testGetHTMLSubject(string $richSubject, string $parsedSubject, array $richParams, string $expected): void {
404+
$event = $this->createMock(IEvent::class);
405+
$event->method('getRichSubject')->willReturn($richSubject);
406+
$event->method('getParsedSubject')->willReturn($parsedSubject);
407+
$event->method('getRichSubjectParameters')->willReturn($richParams);
408+
409+
$result = self::invokePrivate($this->mailQueueHandler, 'getHTMLSubject', [$event]);
410+
$this->assertSame($expected, $result);
411+
}
412+
378413
protected function assertRemainingMailEntries(array $users, int $maxTime, string $explain): void {
379414
foreach ($users as $user) {
380415
[$data,] = self::invokePrivate($this->mailQueueHandler, 'getItemsForUser', [$user, $maxTime]);

0 commit comments

Comments
 (0)