Restrict notification action links to application paths
This commit is contained in:
@@ -3,12 +3,28 @@ from __future__ import annotations
|
|||||||
from datetime import datetime
|
from datetime import datetime
|
||||||
from typing import Any, Literal
|
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"]
|
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):
|
class NotificationCreateRequest(BaseModel):
|
||||||
model_config = ConfigDict(extra="forbid")
|
model_config = ConfigDict(extra="forbid")
|
||||||
|
|
||||||
@@ -31,6 +47,11 @@ class NotificationCreateRequest(BaseModel):
|
|||||||
payload: dict[str, Any] = Field(default_factory=dict)
|
payload: dict[str, Any] = Field(default_factory=dict)
|
||||||
metadata: 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):
|
class NotificationUpdateRequest(BaseModel):
|
||||||
model_config = ConfigDict(extra="forbid")
|
model_config = ConfigDict(extra="forbid")
|
||||||
|
|||||||
@@ -12,7 +12,12 @@ from sqlalchemy.orm import Session
|
|||||||
from govoplan_core.core.notifications import NotificationDispatchRequest
|
from govoplan_core.core.notifications import NotificationDispatchRequest
|
||||||
from govoplan_core.db.base import utcnow
|
from govoplan_core.db.base import utcnow
|
||||||
from govoplan_notifications.backend.db.models import NotificationDeliveryAttempt, NotificationMessage, NotificationPreference
|
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):
|
class NotificationError(ValueError):
|
||||||
@@ -81,6 +86,15 @@ def _clean_source_modules(values: list[str]) -> list[str]:
|
|||||||
return cleaned
|
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(
|
def create_notification(
|
||||||
session: Session,
|
session: Session,
|
||||||
*,
|
*,
|
||||||
@@ -101,7 +115,7 @@ def create_notification(
|
|||||||
subject=payload.subject,
|
subject=payload.subject,
|
||||||
body_text=payload.body_text,
|
body_text=payload.body_text,
|
||||||
body_html=payload.body_html,
|
body_html=payload.body_html,
|
||||||
action_url=payload.action_url,
|
action_url=normalize_notification_action_url(payload.action_url),
|
||||||
priority=payload.priority,
|
priority=payload.priority,
|
||||||
not_before_at=payload.not_before_at,
|
not_before_at=payload.not_before_at,
|
||||||
status="queued" if payload.enqueue_delivery else "pending",
|
status="queued" if payload.enqueue_delivery else "pending",
|
||||||
@@ -485,7 +499,7 @@ def notification_response(notification: NotificationMessage) -> dict[str, Any]:
|
|||||||
"subject": notification.subject,
|
"subject": notification.subject,
|
||||||
"body_text": notification.body_text,
|
"body_text": notification.body_text,
|
||||||
"body_html": notification.body_html,
|
"body_html": notification.body_html,
|
||||||
"action_url": notification.action_url,
|
"action_url": _safe_notification_action_url(notification.action_url),
|
||||||
"priority": notification.priority,
|
"priority": notification.priority,
|
||||||
"status": notification.status,
|
"status": notification.status,
|
||||||
"not_before_at": response_datetime(notification.not_before_at),
|
"not_before_at": response_datetime(notification.not_before_at),
|
||||||
|
|||||||
@@ -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.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.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.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):
|
class NotificationServiceTests(unittest.TestCase):
|
||||||
@@ -187,6 +194,53 @@ class NotificationServiceTests(unittest.TestCase):
|
|||||||
session.commit()
|
session.commit()
|
||||||
enqueue.assert_not_called()
|
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:
|
def test_delivery_task_is_enqueued_only_after_notification_commit(self) -> None:
|
||||||
with patch("govoplan_notifications.backend.service._enqueue_celery_delivery") as enqueue:
|
with patch("govoplan_notifications.backend.service._enqueue_celery_delivery") as enqueue:
|
||||||
with self.Session() as session:
|
with self.Session() as session:
|
||||||
|
|||||||
@@ -13,6 +13,9 @@
|
|||||||
},
|
},
|
||||||
"./styles/notifications.css": "./src/styles/notifications.css"
|
"./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": {
|
"peerDependencies": {
|
||||||
"@govoplan/core-webui": "^0.1.8",
|
"@govoplan/core-webui": "^0.1.8",
|
||||||
"lucide-react": "^1.23.0",
|
"lucide-react": "^1.23.0",
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import { useEffect, useMemo, useState } from "react";
|
|||||||
import { Bell, Check, ExternalLink, RefreshCw, Send, XCircle } from "lucide-react";
|
import { Bell, Check, ExternalLink, RefreshCw, Send, XCircle } from "lucide-react";
|
||||||
import { Button, DismissibleAlert, hasScope, type ApiSettings, type AuthInfo } from "@govoplan/core-webui";
|
import { Button, DismissibleAlert, hasScope, type ApiSettings, type AuthInfo } from "@govoplan/core-webui";
|
||||||
import { deliverPendingNotifications, listNotifications, updateNotification, type NotificationMessage } from "../../api/notifications";
|
import { deliverPendingNotifications, listNotifications, updateNotification, type NotificationMessage } from "../../api/notifications";
|
||||||
|
import { safeNotificationActionUrl } from "../../security/actionUrl";
|
||||||
|
|
||||||
type StatusFilter = "all" | "pending" | "queued" | "sent" | "failed" | "skipped" | "cancelled";
|
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 }) {
|
function NotificationDetails({ notification }: { notification: NotificationMessage }) {
|
||||||
|
const actionUrl = safeNotificationActionUrl(notification.action_url);
|
||||||
return (
|
return (
|
||||||
<div className="notifications-detail">
|
<div className="notifications-detail">
|
||||||
<section className="notifications-message">
|
<section className="notifications-message">
|
||||||
@@ -181,8 +183,8 @@ function NotificationDetails({ notification }: { notification: NotificationMessa
|
|||||||
</div>
|
</div>
|
||||||
<h1>{notification.subject || notification.event_kind}</h1>
|
<h1>{notification.subject || notification.event_kind}</h1>
|
||||||
{notification.body_text ? <p>{notification.body_text}</p> : <p className="muted">No message body was provided.</p>}
|
{notification.body_text ? <p>{notification.body_text}</p> : <p className="muted">No message body was provided.</p>}
|
||||||
{notification.action_url ? (
|
{actionUrl ? (
|
||||||
<a className="notifications-action-link" href={notification.action_url}>
|
<a className="notifications-action-link" href={actionUrl}>
|
||||||
<ExternalLink size={16} /> Open related item
|
<ExternalLink size={16} /> Open related item
|
||||||
</a>
|
</a>
|
||||||
) : null}
|
) : null}
|
||||||
|
|||||||
16
webui/src/security/actionUrl.ts
Normal file
16
webui/src/security/actionUrl.ts
Normal file
@@ -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;
|
||||||
|
}
|
||||||
27
webui/tests/action-url.test.ts
Normal file
27
webui/tests/action-url.test.ts
Normal file
@@ -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");
|
||||||
16
webui/tsconfig.tests.json
Normal file
16
webui/tsconfig.tests.json
Normal file
@@ -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"
|
||||||
|
]
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user