Skip to content

Commit eecd708

Browse files
nickvergessennickvergessen
authored
Merge pull request #59606 from nextcloud/bugfix/noid/require-absolute-links-in-notifications
fix(notifications): Require absolute links for support of desktop and mobile clients
2 parents 2b76471 + 36e956e commit eecd708

7 files changed

Lines changed: 88 additions & 50 deletions

File tree

‎lib/private/Notification/Action.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,12 @@ public function setLink(string $link, string $requestType): IAction {
7676
if ($link === '' || isset($link[256])) {
7777
throw new InvalidValueException('link');
7878
}
79+
80+
// Only allow absolute URLs for support of desktop and mobile clients
81+
if (!str_starts_with($link, 'http://') && !str_starts_with($link, 'https://')) {
82+
throw new InvalidValueException('link');
83+
}
84+
7985
if (!in_array($requestType, [
8086
self::TYPE_GET,
8187
self::TYPE_POST,

‎lib/private/Notification/Manager.php‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -375,23 +375,6 @@ public function prepare(INotification $notification, string $languageCode): INot
375375
throw new IncompleteParsedNotificationException();
376376
}
377377

378-
$link = $notification->getLink();
379-
if ($link !== '' && !str_starts_with($link, 'http://') && !str_starts_with($link, 'https://')) {
380-
$this->logger->warning('Link of notification is not an absolute URL and does not work in mobile and desktop clients [app: ' . $notification->getApp() . ', subject: ' . $notification->getSubject() . ']');
381-
}
382-
383-
$icon = $notification->getIcon();
384-
if ($icon !== '' && !str_starts_with($icon, 'http://') && !str_starts_with($icon, 'https://')) {
385-
$this->logger->warning('Icon of notification is not an absolute URL and does not work in mobile and desktop clients [app: ' . $notification->getApp() . ', subject: ' . $notification->getSubject() . ']');
386-
}
387-
388-
foreach ($notification->getParsedActions() as $action) {
389-
$link = $action->getLink();
390-
if ($link !== '' && !str_starts_with($link, 'http://') && !str_starts_with($link, 'https://')) {
391-
$this->logger->warning('Link of action is not an absolute URL and does not work in mobile and desktop clients [app: ' . $notification->getApp() . ', subject: ' . $notification->getSubject() . ']');
392-
}
393-
}
394-
395378
return $notification;
396379
}
397380

‎lib/private/Notification/Notification.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,12 @@ public function setLink(string $link): INotification {
310310
if ($link === '' || isset($link[4000])) {
311311
throw new InvalidValueException('link');
312312
}
313+
314+
// Only allow absolute URLs for support of desktop and mobile clients
315+
if (!str_starts_with($link, 'http://') && !str_starts_with($link, 'https://')) {
316+
throw new InvalidValueException('link');
317+
}
318+
313319
$this->link = $link;
314320
return $this;
315321
}
@@ -328,6 +334,12 @@ public function setIcon(string $icon): INotification {
328334
if ($icon === '' || isset($icon[4000])) {
329335
throw new InvalidValueException('icon');
330336
}
337+
338+
// Only allow absolute URLs for support of desktop and mobile clients
339+
if (!str_starts_with($icon, 'http://') && !str_starts_with($icon, 'https://')) {
340+
throw new InvalidValueException('icon');
341+
}
342+
331343
$this->icon = $icon;
332344
return $this;
333345
}

‎lib/public/Notification/IAction.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,10 @@ public function setPrimary(bool $primary): IAction;
7777
public function isPrimary(): bool;
7878

7979
/**
80+
* Set the target endpoint for this action
81+
*
82+
* All links should always be relative to support desktop and mobile clients.
83+
*
8084
* @param string $link
8185
* @param string $requestType
8286
* @return $this

‎lib/public/Notification/INotification.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,10 @@ public function getRichMessage(): string;
232232
public function getRichMessageParameters(): array;
233233

234234
/**
235+
* Set the target endpoint for this action
236+
*
237+
* All links should always be relative to support desktop and mobile clients.
238+
*
235239
* @param string $link
236240
* @return $this
237241
* @throws InvalidValueException if the link is invalid

‎tests/lib/Notification/ActionTest.php‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,13 @@ public function testSetParsedLabelInvalid($label): void {
9494

9595
public static function dataSetLink(): array {
9696
return [
97-
['test1', 'GET'],
98-
['test2', 'POST'],
99-
[str_repeat('a', 1), 'PUT'],
100-
[str_repeat('a', 256), 'DELETE'],
97+
['http://example.tld/', 'GET'],
98+
['https://example.tld/api/v1/resource', 'POST'],
99+
['https://example.tld/?q=1&r=2', 'PUT'],
100+
['https://example.tld/path#frag', 'DELETE'],
101+
['https://example.tld/web', 'WEB'],
102+
// Maximum length (256 chars total, including the scheme)
103+
['https://' . str_repeat('a', 256 - 8), 'GET'],
101104
];
102105
}
103106

@@ -115,12 +118,27 @@ public function testSetLink($link, $type): void {
115118

116119
public static function dataSetLinkInvalid(): array {
117120
return [
118-
// Invalid link
121+
// Invalid link — empty / too long
119122
['', 'GET'],
120-
[str_repeat('a', 257), 'GET'],
121-
122-
// Invalid type
123-
['url', 'notGET'],
123+
['https://' . str_repeat('a', 257 - 8), 'GET'],
124+
125+
// Disallowed schemes
126+
['javascript:alert(1)', 'WEB'],
127+
['JavaScript:alert(1)', 'WEB'],
128+
['javascript:alert(1)', 'GET'],
129+
['data:text/html,<script>alert(1)</script>', 'WEB'],
130+
['vbscript:msgbox("xss")', 'WEB'],
131+
['file:///etc/passwd', 'GET'],
132+
['mailto:test@example.tld', 'WEB'],
133+
['ftp://example.tld/', 'GET'],
134+
135+
// Relative urls
136+
['/relative/path', 'WEB'],
137+
['//protocol-relative.tld/', 'GET'],
138+
['url', 'GET'],
139+
140+
// Invalid type (with a valid http(s) link, so the type check is what trips)
141+
['https://example.tld/', 'notGET'],
124142
];
125143
}
126144

@@ -159,7 +177,7 @@ public function testIsValid(): void {
159177
$this->action->setLabel('label');
160178
$this->assertFalse($this->action->isValid());
161179
$this->assertFalse($this->action->isValidParsed());
162-
$this->action->setLink('link', 'GET');
180+
$this->action->setLink('https://example.tld/', 'GET');
163181
$this->assertTrue($this->action->isValid());
164182
$this->assertFalse($this->action->isValidParsed());
165183
}
@@ -170,7 +188,7 @@ public function testIsValidParsed(): void {
170188
$this->action->setParsedLabel('label');
171189
$this->assertFalse($this->action->isValid());
172190
$this->assertFalse($this->action->isValidParsed());
173-
$this->action->setLink('link', 'GET');
191+
$this->action->setLink('https://example.tld/', 'GET');
174192
$this->assertFalse($this->action->isValid());
175193
$this->assertTrue($this->action->isValidParsed());
176194
}

‎tests/lib/Notification/NotificationTest.php‎

Lines changed: 33 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,13 @@ public function testSetParsedMessageInvalid($message): void {
327327
}
328328

329329
public static function dataSetLink(): array {
330-
return self::dataValidString(4000);
330+
return [
331+
['http://example.tld/'],
332+
['https://example.tld/'],
333+
['https://example.tld/path/to/resource?query=1&other=2#fragment'],
334+
// Maximum length (4000 chars total, including the scheme)
335+
['https://' . str_repeat('a', 4000 - 8)],
336+
];
331337
}
332338

333339
/**
@@ -341,29 +347,38 @@ public function testSetLink($link): void {
341347
}
342348

343349
public static function dataSetLinkInvalid(): array {
344-
return self::dataInvalidString(4000);
350+
return [
351+
// Empty / too long
352+
[''],
353+
['https://' . str_repeat('a', 4001 - 8)],
354+
355+
// Disallowed schemes
356+
['javascript:alert(1)'],
357+
['JavaScript:alert(1)'],
358+
['data:text/html,<script>alert(1)</script>'],
359+
['vbscript:msgbox("xss")'],
360+
['file:///etc/passwd'],
361+
['mailto:test@example.tld'],
362+
['ftp://example.tld/'],
363+
['ws://example.tld/'],
364+
365+
// Relative urls
366+
['/relative/path'],
367+
['//protocol-relative.tld/'],
368+
['example.tld/path'],
369+
['test1'],
370+
];
345371
}
346372

347-
/**
348-
* @param mixed $link
349-
*
350-
*/
351373
#[\PHPUnit\Framework\Attributes\DataProvider('dataSetLinkInvalid')]
352-
public function testSetLinkInvalid($link): void {
374+
public function testSetLinkInvalid(string $link): void {
353375
$this->expectException(\InvalidArgumentException::class);
354376

355377
$this->notification->setLink($link);
356378
}
357379

358-
public static function dataSetIcon(): array {
359-
return self::dataValidString(4000);
360-
}
361-
362-
/**
363-
* @param string $icon
364-
*/
365-
#[\PHPUnit\Framework\Attributes\DataProvider('dataSetIcon')]
366-
public function testSetIcon($icon): void {
380+
#[\PHPUnit\Framework\Attributes\DataProvider('dataSetLink')]
381+
public function testSetIcon(string $icon): void {
367382
$this->assertSame('', $this->notification->getIcon());
368383
$this->assertSame($this->notification, $this->notification->setIcon($icon));
369384
$this->assertSame($icon, $this->notification->getIcon());
@@ -373,12 +388,8 @@ public static function dataSetIconInvalid(): array {
373388
return self::dataInvalidString(4000);
374389
}
375390

376-
/**
377-
* @param mixed $icon
378-
*
379-
*/
380-
#[\PHPUnit\Framework\Attributes\DataProvider('dataSetIconInvalid')]
381-
public function testSetIconInvalid($icon): void {
391+
#[\PHPUnit\Framework\Attributes\DataProvider('dataSetLinkInvalid')]
392+
public function testSetIconInvalid(string $icon): void {
382393
$this->expectException(\InvalidArgumentException::class);
383394

384395
$this->notification->setIcon($icon);

0 commit comments

Comments
 (0)