diff --git a/lib/private/Notification/Action.php b/lib/private/Notification/Action.php index e2a75bea03080..0ae86760a32ef 100644 --- a/lib/private/Notification/Action.php +++ b/lib/private/Notification/Action.php @@ -76,6 +76,12 @@ public function setLink(string $link, string $requestType): IAction { if ($link === '' || isset($link[256])) { throw new InvalidValueException('link'); } + + // Only allow absolute URLs for support of desktop and mobile clients + if (!str_starts_with($link, 'http://') && !str_starts_with($link, 'https://')) { + throw new InvalidValueException('link'); + } + if (!in_array($requestType, [ self::TYPE_GET, self::TYPE_POST, diff --git a/lib/private/Notification/Manager.php b/lib/private/Notification/Manager.php index 62fa66df85cd6..5fdd5b2cb4d39 100644 --- a/lib/private/Notification/Manager.php +++ b/lib/private/Notification/Manager.php @@ -372,23 +372,6 @@ public function prepare(INotification $notification, string $languageCode): INot throw new IncompleteParsedNotificationException(); } - $link = $notification->getLink(); - if ($link !== '' && !str_starts_with($link, 'http://') && !str_starts_with($link, 'https://')) { - $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() . ']'); - } - - $icon = $notification->getIcon(); - if ($icon !== '' && !str_starts_with($icon, 'http://') && !str_starts_with($icon, 'https://')) { - $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() . ']'); - } - - foreach ($notification->getParsedActions() as $action) { - $link = $action->getLink(); - if ($link !== '' && !str_starts_with($link, 'http://') && !str_starts_with($link, 'https://')) { - $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() . ']'); - } - } - return $notification; } diff --git a/lib/private/Notification/Notification.php b/lib/private/Notification/Notification.php index fcce7fd00201d..07d7aef363faf 100644 --- a/lib/private/Notification/Notification.php +++ b/lib/private/Notification/Notification.php @@ -310,6 +310,12 @@ public function setLink(string $link): INotification { if ($link === '' || isset($link[4000])) { throw new InvalidValueException('link'); } + + // Only allow absolute URLs for support of desktop and mobile clients + if (!str_starts_with($link, 'http://') && !str_starts_with($link, 'https://')) { + throw new InvalidValueException('link'); + } + $this->link = $link; return $this; } @@ -328,6 +334,12 @@ public function setIcon(string $icon): INotification { if ($icon === '' || isset($icon[4000])) { throw new InvalidValueException('icon'); } + + // Only allow absolute URLs for support of desktop and mobile clients + if (!str_starts_with($icon, 'http://') && !str_starts_with($icon, 'https://')) { + throw new InvalidValueException('icon'); + } + $this->icon = $icon; return $this; } diff --git a/lib/public/Notification/IAction.php b/lib/public/Notification/IAction.php index 722dac72826fb..3c33ee1d72763 100644 --- a/lib/public/Notification/IAction.php +++ b/lib/public/Notification/IAction.php @@ -77,6 +77,10 @@ public function setPrimary(bool $primary): IAction; public function isPrimary(): bool; /** + * Set the target endpoint for this action + * + * All links should always be relative to support desktop and mobile clients. + * * @param string $link * @param string $requestType * @return $this diff --git a/lib/public/Notification/INotification.php b/lib/public/Notification/INotification.php index a740678376fbb..281fe24faf46c 100644 --- a/lib/public/Notification/INotification.php +++ b/lib/public/Notification/INotification.php @@ -232,6 +232,10 @@ public function getRichMessage(): string; public function getRichMessageParameters(): array; /** + * Set the target endpoint for this action + * + * All links should always be relative to support desktop and mobile clients. + * * @param string $link * @return $this * @throws InvalidValueException if the link is invalid diff --git a/tests/lib/Notification/ActionTest.php b/tests/lib/Notification/ActionTest.php index 252819f1e295c..0ff364a9e65bf 100644 --- a/tests/lib/Notification/ActionTest.php +++ b/tests/lib/Notification/ActionTest.php @@ -94,10 +94,13 @@ public function testSetParsedLabelInvalid($label): void { public static function dataSetLink(): array { return [ - ['test1', 'GET'], - ['test2', 'POST'], - [str_repeat('a', 1), 'PUT'], - [str_repeat('a', 256), 'DELETE'], + ['http://example.tld/', 'GET'], + ['https://example.tld/api/v1/resource', 'POST'], + ['https://example.tld/?q=1&r=2', 'PUT'], + ['https://example.tld/path#frag', 'DELETE'], + ['https://example.tld/web', 'WEB'], + // Maximum length (256 chars total, including the scheme) + ['https://' . str_repeat('a', 256 - 8), 'GET'], ]; } @@ -115,12 +118,27 @@ public function testSetLink($link, $type): void { public static function dataSetLinkInvalid(): array { return [ - // Invalid link + // Invalid link — empty / too long ['', 'GET'], - [str_repeat('a', 257), 'GET'], - - // Invalid type - ['url', 'notGET'], + ['https://' . str_repeat('a', 257 - 8), 'GET'], + + // Disallowed schemes + ['javascript:alert(1)', 'WEB'], + ['JavaScript:alert(1)', 'WEB'], + ['javascript:alert(1)', 'GET'], + ['data:text/html,', 'WEB'], + ['vbscript:msgbox("xss")', 'WEB'], + ['file:///etc/passwd', 'GET'], + ['mailto:test@example.tld', 'WEB'], + ['ftp://example.tld/', 'GET'], + + // Relative urls + ['/relative/path', 'WEB'], + ['//protocol-relative.tld/', 'GET'], + ['url', 'GET'], + + // Invalid type (with a valid http(s) link, so the type check is what trips) + ['https://example.tld/', 'notGET'], ]; } @@ -159,7 +177,7 @@ public function testIsValid(): void { $this->action->setLabel('label'); $this->assertFalse($this->action->isValid()); $this->assertFalse($this->action->isValidParsed()); - $this->action->setLink('link', 'GET'); + $this->action->setLink('https://example.tld/', 'GET'); $this->assertTrue($this->action->isValid()); $this->assertFalse($this->action->isValidParsed()); } @@ -170,7 +188,7 @@ public function testIsValidParsed(): void { $this->action->setParsedLabel('label'); $this->assertFalse($this->action->isValid()); $this->assertFalse($this->action->isValidParsed()); - $this->action->setLink('link', 'GET'); + $this->action->setLink('https://example.tld/', 'GET'); $this->assertFalse($this->action->isValid()); $this->assertTrue($this->action->isValidParsed()); } diff --git a/tests/lib/Notification/NotificationTest.php b/tests/lib/Notification/NotificationTest.php index cebe9d51021e3..b09ff2085d448 100644 --- a/tests/lib/Notification/NotificationTest.php +++ b/tests/lib/Notification/NotificationTest.php @@ -327,7 +327,13 @@ public function testSetParsedMessageInvalid($message): void { } public static function dataSetLink(): array { - return self::dataValidString(4000); + return [ + ['http://example.tld/'], + ['https://example.tld/'], + ['https://example.tld/path/to/resource?query=1&other=2#fragment'], + // Maximum length (4000 chars total, including the scheme) + ['https://' . str_repeat('a', 4000 - 8)], + ]; } /** @@ -341,29 +347,38 @@ public function testSetLink($link): void { } public static function dataSetLinkInvalid(): array { - return self::dataInvalidString(4000); + return [ + // Empty / too long + [''], + ['https://' . str_repeat('a', 4001 - 8)], + + // Disallowed schemes + ['javascript:alert(1)'], + ['JavaScript:alert(1)'], + ['data:text/html,'], + ['vbscript:msgbox("xss")'], + ['file:///etc/passwd'], + ['mailto:test@example.tld'], + ['ftp://example.tld/'], + ['ws://example.tld/'], + + // Relative urls + ['/relative/path'], + ['//protocol-relative.tld/'], + ['example.tld/path'], + ['test1'], + ]; } - /** - * @param mixed $link - * - */ #[\PHPUnit\Framework\Attributes\DataProvider('dataSetLinkInvalid')] - public function testSetLinkInvalid($link): void { + public function testSetLinkInvalid(string $link): void { $this->expectException(\InvalidArgumentException::class); $this->notification->setLink($link); } - public static function dataSetIcon(): array { - return self::dataValidString(4000); - } - - /** - * @param string $icon - */ - #[\PHPUnit\Framework\Attributes\DataProvider('dataSetIcon')] - public function testSetIcon($icon): void { + #[\PHPUnit\Framework\Attributes\DataProvider('dataSetLink')] + public function testSetIcon(string $icon): void { $this->assertSame('', $this->notification->getIcon()); $this->assertSame($this->notification, $this->notification->setIcon($icon)); $this->assertSame($icon, $this->notification->getIcon()); @@ -373,12 +388,8 @@ public static function dataSetIconInvalid(): array { return self::dataInvalidString(4000); } - /** - * @param mixed $icon - * - */ - #[\PHPUnit\Framework\Attributes\DataProvider('dataSetIconInvalid')] - public function testSetIconInvalid($icon): void { + #[\PHPUnit\Framework\Attributes\DataProvider('dataSetLinkInvalid')] + public function testSetIconInvalid(string $icon): void { $this->expectException(\InvalidArgumentException::class); $this->notification->setIcon($icon);