From ae144a13e034013d6f140052c68caf6fe7c7c5cd Mon Sep 17 00:00:00 2001 From: Albrecht Degering Date: Tue, 21 Jul 2026 03:16:51 +0200 Subject: [PATCH] Restrict notification action links to application paths --- src/govoplan_notifications/backend/schemas.py | 23 +++++++- src/govoplan_notifications/backend/service.py | 20 ++++++- tests/test_notifications.py | 56 ++++++++++++++++++- webui/package.json | 3 + .../notifications/NotificationCenterPage.tsx | 6 +- webui/src/security/actionUrl.ts | 16 ++++++ webui/tests/action-url.test.ts | 27 +++++++++ webui/tsconfig.tests.json | 16 ++++++ 8 files changed, 160 insertions(+), 7 deletions(-) create mode 100644 webui/src/security/actionUrl.ts create mode 100644 webui/tests/action-url.test.ts create mode 100644 webui/tsconfig.tests.json diff --git a/src/govoplan_notifications/backend/schemas.py b/src/govoplan_notifications/backend/schemas.py index 5b1958d..84e9132 100644 --- a/src/govoplan_notifications/backend/schemas.py +++ b/src/govoplan_notifications/backend/schemas.py @@ -3,12 +3,28 @@ from __future__ import annotations from datetime import datetime from typing import Any, Literal -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, field_validator NotificationStatus = Literal["pending", "queued", "sending", "sent", "failed", "skipped", "cancelled"] +def normalize_notification_action_url(value: str | None) -> str | None: + if value is None: + return None + candidate = value.strip() + if not candidate: + return None + if ( + not candidate.startswith("/") + or candidate.startswith("//") + or "\\" in candidate + or any(ord(character) < 32 or ord(character) == 127 for character in candidate) + ): + raise ValueError("Notification action URL must be an application-relative path") + return candidate + + class NotificationCreateRequest(BaseModel): model_config = ConfigDict(extra="forbid") @@ -31,6 +47,11 @@ class NotificationCreateRequest(BaseModel): payload: dict[str, Any] = Field(default_factory=dict) metadata: dict[str, Any] = Field(default_factory=dict) + @field_validator("action_url") + @classmethod + def validate_action_url(cls, value: str | None) -> str | None: + return normalize_notification_action_url(value) + class NotificationUpdateRequest(BaseModel): model_config = ConfigDict(extra="forbid") diff --git a/src/govoplan_notifications/backend/service.py b/src/govoplan_notifications/backend/service.py index fd663b8..0c5e6d7 100644 --- a/src/govoplan_notifications/backend/service.py +++ b/src/govoplan_notifications/backend/service.py @@ -12,7 +12,12 @@ from sqlalchemy.orm import Session from govoplan_core.core.notifications import NotificationDispatchRequest from govoplan_core.db.base import utcnow from govoplan_notifications.backend.db.models import NotificationDeliveryAttempt, NotificationMessage, NotificationPreference -from govoplan_notifications.backend.schemas import NotificationCreateRequest, NotificationPreferencesUpdateRequest, NotificationUpdateRequest +from govoplan_notifications.backend.schemas import ( + NotificationCreateRequest, + NotificationPreferencesUpdateRequest, + NotificationUpdateRequest, + normalize_notification_action_url, +) class NotificationError(ValueError): @@ -81,6 +86,15 @@ def _clean_source_modules(values: list[str]) -> list[str]: return cleaned +def _safe_notification_action_url(value: str | None) -> str | None: + try: + return normalize_notification_action_url(value) + except ValueError: + # Legacy rows predate action URL validation. Never project an unsafe + # stored value back into a clickable browser link. + return None + + def create_notification( session: Session, *, @@ -101,7 +115,7 @@ def create_notification( subject=payload.subject, body_text=payload.body_text, body_html=payload.body_html, - action_url=payload.action_url, + action_url=normalize_notification_action_url(payload.action_url), priority=payload.priority, not_before_at=payload.not_before_at, status="queued" if payload.enqueue_delivery else "pending", @@ -485,7 +499,7 @@ def notification_response(notification: NotificationMessage) -> dict[str, Any]: "subject": notification.subject, "body_text": notification.body_text, "body_html": notification.body_html, - "action_url": notification.action_url, + "action_url": _safe_notification_action_url(notification.action_url), "priority": notification.priority, "status": notification.status, "not_before_at": response_datetime(notification.not_before_at), diff --git a/tests/test_notifications.py b/tests/test_notifications.py index 0e954dd..a889be3 100644 --- a/tests/test_notifications.py +++ b/tests/test_notifications.py @@ -18,7 +18,14 @@ from govoplan_notifications.backend.capabilities import dispatch_capability from govoplan_notifications.backend.db.models import NotificationDeliveryAttempt, NotificationMessage, NotificationPreference from govoplan_notifications.backend.router import api_get_notification, api_list_notifications, api_notification_summary, api_update_notification from govoplan_notifications.backend.schemas import NotificationCreateRequest, NotificationPreferencesUpdateRequest, NotificationUpdateRequest -from govoplan_notifications.backend.service import create_notification, deliver_pending, notification_preferences_response, notification_summary, update_notification_preferences +from govoplan_notifications.backend.service import ( + create_notification, + deliver_pending, + notification_preferences_response, + notification_response, + notification_summary, + update_notification_preferences, +) class NotificationServiceTests(unittest.TestCase): @@ -187,6 +194,53 @@ class NotificationServiceTests(unittest.TestCase): session.commit() enqueue.assert_not_called() + def test_action_url_accepts_only_application_relative_paths(self) -> None: + payload = self._inbox_payload(recipient_id="user-1", subject="Safe action") + safe = payload.model_copy(update={"action_url": "/calendar?event=event-1#details"}) + self.assertEqual( + NotificationCreateRequest.model_validate(safe.model_dump()).action_url, + "/calendar?event=event-1#details", + ) + + for action_url in ( + "javascript:alert(1)", + "data:text/html,unsafe", + "file:///etc/passwd", + "custom:unsafe", + "https://attacker.example.test/collect", + "//attacker.example.test/collect", + "/\\attacker.example.test/collect", + "/calendar\nmalformed", + ): + with self.subTest(action_url=action_url): + with self.assertRaisesRegex(ValueError, "application-relative path"): + NotificationCreateRequest.model_validate( + {**payload.model_dump(), "action_url": action_url} + ) + + bypassed_validation = NotificationCreateRequest.model_construct( + **{**payload.model_dump(), "action_url": "javascript:alert(1)"} + ) + with self.Session() as session: + with self.assertRaisesRegex(ValueError, "application-relative path"): + create_notification( + session, + tenant_id="tenant-1", + payload=bypassed_validation, + ) + + def test_legacy_unsafe_action_url_is_not_projected_as_a_link(self) -> None: + with self.Session() as session: + notification = create_notification( + session, + tenant_id="tenant-1", + payload=self._inbox_payload(recipient_id="user-1", subject="Legacy action"), + ) + notification.action_url = "javascript:alert(1)" + session.flush() + + self.assertIsNone(notification_response(notification)["action_url"]) + def test_delivery_task_is_enqueued_only_after_notification_commit(self) -> None: with patch("govoplan_notifications.backend.service._enqueue_celery_delivery") as enqueue: with self.Session() as session: diff --git a/webui/package.json b/webui/package.json index 4226e8c..da668d8 100644 --- a/webui/package.json +++ b/webui/package.json @@ -13,6 +13,9 @@ }, "./styles/notifications.css": "./src/styles/notifications.css" }, + "scripts": { + "test:action-url": "rm -rf .component-test-build && mkdir -p .component-test-build && printf '{\"type\":\"commonjs\"}\\n' > .component-test-build/package.json && tsc -p tsconfig.tests.json && node .component-test-build/tests/action-url.test.js" + }, "peerDependencies": { "@govoplan/core-webui": "^0.1.8", "lucide-react": "^1.23.0", diff --git a/webui/src/features/notifications/NotificationCenterPage.tsx b/webui/src/features/notifications/NotificationCenterPage.tsx index acf4ea8..d1ed035 100644 --- a/webui/src/features/notifications/NotificationCenterPage.tsx +++ b/webui/src/features/notifications/NotificationCenterPage.tsx @@ -2,6 +2,7 @@ import { useEffect, useMemo, useState } from "react"; import { Bell, Check, ExternalLink, RefreshCw, Send, XCircle } from "lucide-react"; import { Button, DismissibleAlert, hasScope, type ApiSettings, type AuthInfo } from "@govoplan/core-webui"; import { deliverPendingNotifications, listNotifications, updateNotification, type NotificationMessage } from "../../api/notifications"; +import { safeNotificationActionUrl } from "../../security/actionUrl"; type StatusFilter = "all" | "pending" | "queued" | "sent" | "failed" | "skipped" | "cancelled"; @@ -171,6 +172,7 @@ export default function NotificationCenterPage({ settings, auth }: { settings: A } function NotificationDetails({ notification }: { notification: NotificationMessage }) { + const actionUrl = safeNotificationActionUrl(notification.action_url); return (
@@ -181,8 +183,8 @@ function NotificationDetails({ notification }: { notification: NotificationMessa

{notification.subject || notification.event_kind}

{notification.body_text ?

{notification.body_text}

:

No message body was provided.

} - {notification.action_url ? ( - + {actionUrl ? ( + Open related item ) : null} diff --git a/webui/src/security/actionUrl.ts b/webui/src/security/actionUrl.ts new file mode 100644 index 0000000..773e710 --- /dev/null +++ b/webui/src/security/actionUrl.ts @@ -0,0 +1,16 @@ +export function safeNotificationActionUrl(value: string | null | undefined): string | null { + const candidate = value?.trim(); + if ( + !candidate + || !candidate.startsWith("/") + || candidate.startsWith("//") + || candidate.includes("\\") + || [...candidate].some((character) => { + const codePoint = character.codePointAt(0) ?? 0; + return codePoint < 32 || codePoint === 127; + }) + ) { + return null; + } + return candidate; +} diff --git a/webui/tests/action-url.test.ts b/webui/tests/action-url.test.ts new file mode 100644 index 0000000..c6dbcbb --- /dev/null +++ b/webui/tests/action-url.test.ts @@ -0,0 +1,27 @@ +import { safeNotificationActionUrl } from "../src/security/actionUrl"; + +function assertEqual(actual: unknown, expected: unknown, message: string): void { + if (actual !== expected) { + throw new Error(`${message}: expected ${String(expected)}, got ${String(actual)}`); + } +} + +assertEqual( + safeNotificationActionUrl(" /calendar?event=event-1#details "), + "/calendar?event=event-1#details", + "application-relative links remain available" +); + +for (const unsafe of [ + "javascript:alert(1)", + "data:text/html,unsafe", + "https://attacker.example.test/collect", + "//attacker.example.test/collect", + "/\\attacker.example.test/collect", + "/calendar\nmalformed", + "/calendar\u007fmalformed" +]) { + assertEqual(safeNotificationActionUrl(unsafe), null, `unsafe link is suppressed: ${unsafe}`); +} + +assertEqual(safeNotificationActionUrl(null), null, "missing links remain absent"); diff --git a/webui/tsconfig.tests.json b/webui/tsconfig.tests.json new file mode 100644 index 0000000..eaa7e3e --- /dev/null +++ b/webui/tsconfig.tests.json @@ -0,0 +1,16 @@ +{ + "compilerOptions": { + "target": "ES2022", + "module": "CommonJS", + "moduleResolution": "Node", + "strict": true, + "esModuleInterop": true, + "skipLibCheck": true, + "outDir": ".component-test-build", + "rootDir": "." + }, + "include": [ + "src/security/actionUrl.ts", + "tests/action-url.test.ts" + ] +}