Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions openwisp_notifications/api/serializers.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
from urllib.parse import urlparse

from django.db import models
from rest_framework import serializers
Expand Down Expand Up @@ -47,6 +48,20 @@ def get_field_names(self, declared_fields, info):
model_fields = super().get_field_names(declared_fields, info)
return model_fields + self.Meta.extra_fields

def to_representation(self, instance):
data = super().to_representation(instance)
url = (instance.data or {}).get("url")
if url and self._is_safe_target_url(url):
data["target_url"] = url
return data
Comment on lines +51 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject non-http(s) url overrides in to_representation.

instance.data["url"] is returned as target_url without validating the URL scheme. A crafted javascript: URL in notification data would be passed to the frontend and could execute on click. Only allow http/https schemes and fall back to the base target_url otherwise.

🛡️ Proposed fix: validate URL scheme before overriding
    def to_representation(self, instance):
        data = super().to_representation(instance)
-        if instance.data and instance.data.get("url"):
-            data["target_url"] = instance.data["url"]
+        if instance.data:
+            url = instance.data.get("url")
+            if url and url.startswith(("http://", "https://")):
+                data["target_url"] = url
        return data
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_notifications/api/serializers.py` around lines 50 - 54, Update
to_representation to validate instance.data["url"] before assigning it to
target_url: only accept URLs with http or https schemes, and preserve the
serialized base target_url for all other schemes, including javascript and
malformed values.


@staticmethod
def _is_safe_target_url(url):
"""Allows only relative paths or http(s) URLs as the redirect target."""
if url.startswith("//"):
return False
return urlparse(url).scheme in ("", "http", "https")

@property
def data(self):
try:
Expand Down
37 changes: 37 additions & 0 deletions openwisp_notifications/tests/test_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

from openwisp_notifications import settings as app_settings
from openwisp_notifications import tasks, utils
from openwisp_notifications.api.serializers import NotificationSerializer
from openwisp_notifications.exceptions import NotificationRenderException
from openwisp_notifications.handlers import (
notify_handler,
Expand Down Expand Up @@ -771,6 +772,42 @@ def test_notification_type_related_object_url(self):
notification.target_url, "https://target.example.com/index#heading"
)

def test_target_url_data_override(self):
target = self._create_user(username="target", email="target@example.com")
self.notification_options.update({"target": target})
with self.subTest("data url is used as the notification link"):
self._create_notification()
n = Notification.objects.first()
self.assertEqual(
NotificationSerializer(n).data["target_url"],
"https://localhost:8000/admin",
)
self.assertIn(str(target.pk), n.target_url)
with self.subTest("a relative data url is honored"):
Notification.objects.all().delete()
self.notification_options.update({"url": "/admin/?status=pending"})
self._create_notification()
n = Notification.objects.first()
self.assertEqual(
NotificationSerializer(n).data["target_url"], "/admin/?status=pending"
)
with self.subTest("an unsafe scheme falls back to the target object URL"):
for unsafe in ("javascript:alert(1)", "//evil.example.com"):
Notification.objects.all().delete()
self.notification_options.update({"url": unsafe})
self._create_notification()
n = Notification.objects.first()
self.assertEqual(
NotificationSerializer(n).data["target_url"], n.target_url
)
with self.subTest("falls back to the target object URL without a data url"):
Notification.objects.all().delete()
self.notification_options.pop("url")
self._create_notification()
n = Notification.objects.first()
self.assertEqual(NotificationSerializer(n).data["target_url"], n.target_url)
self.assertIn(str(target.pk), n.target_url)

@capture_any_output()
@mock_notification_types
@patch("openwisp_notifications.tasks.delete_notification.delay")
Expand Down
Loading