From f0db4e99aa36e526b32f8601b93f401b75e0089d Mon Sep 17 00:00:00 2001 From: Alvaro Mariano Date: Mon, 22 May 2023 23:26:41 +0000 Subject: [PATCH] #263 Fix vunerability in views --- CHANGELOG.md | 4 ++++ notifications/tests/settings.py | 2 ++ notifications/tests/tests.py | 21 +++++++++++++++++++-- notifications/views.py | 32 +++++++++++++++++--------------- 4 files changed, 42 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d074b52..61c0c58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 1.8.0 + + - #263 Fix vunerability in views + ## 1.7.0 - Added support for Django 3.2 and Django 4.0 diff --git a/notifications/tests/settings.py b/notifications/tests/settings.py index 598b175..7d630d1 100644 --- a/notifications/tests/settings.py +++ b/notifications/tests/settings.py @@ -77,3 +77,5 @@ if os.environ.get('SAMPLE_APP', False): INSTALLED_APPS.append('notifications.tests.sample_notifications') NOTIFICATIONS_NOTIFICATION_MODEL = 'sample_notifications.Notification' TEMPLATES[0]['DIRS'] += [os.path.join(BASE_DIR, '../templates')] + +ALLOWED_HOSTS = [] diff --git a/notifications/tests/tests.py b/notifications/tests/tests.py index e6a07ae..5fbbed0 100644 --- a/notifications/tests/tests.py +++ b/notifications/tests/tests.py @@ -14,7 +14,7 @@ from django.contrib.auth.models import Group, User from django.core.exceptions import ImproperlyConfigured from django.db import connection from django.template import Context, Template -from django.test import RequestFactory, TestCase +from django.test import Client, RequestFactory, TestCase from django.test.utils import CaptureQueriesContext from django.utils import timezone from django.utils.timezone import localtime, utc @@ -39,7 +39,13 @@ except ImportError: # Django <= 1.6 from django.core.urlresolvers import reverse # pylint: disable=no-name-in-module,import-error - +MALICIOUS_NEXT_URLS = [ + "http://bla.com", + "http://www.bla.com", + "https://bla.com", + "https://www.bla.com", + "ftp://www.bla.com/file.exe", +] class NotificationTest(TestCase): ''' Django notifications automated tests ''' @@ -250,6 +256,17 @@ class NotificationTestPages(TestCase): }) self.assertRedirects(response, reverse('notifications:unread') + query_parameters) + @override_settings(ALLOWED_HOSTS=["www.notifications.com"]) + def test_malicious_next_pages(self): + self.client.force_login(self.to_user) + query_parameters = '?var1=hello&var2=world' + + for next_url in MALICIOUS_NEXT_URLS: + response = self.client.get(reverse('notifications:mark_all_as_read'),data={ + "next": next_url + query_parameters, + }) + self.assertRedirects(response, reverse('notifications:unread')) + def test_delete_messages_pages(self): self.login('to', 'pwd') diff --git a/notifications/views.py b/notifications/views.py index a56aaa5..126d325 100644 --- a/notifications/views.py +++ b/notifications/views.py @@ -4,14 +4,16 @@ from distutils.version import \ StrictVersion # pylint: disable=no-name-in-module,import-error from django import get_version +from django.conf import settings from django.contrib.auth.decorators import login_required from django.forms import model_to_dict from django.shortcuts import get_object_or_404, redirect from django.utils.decorators import method_decorator +from django.utils.encoding import iri_to_uri +from django.utils.http import url_has_allowed_host_and_scheme from django.views.decorators.cache import never_cache from django.views.generic import ListView -from notifications import settings -from notifications.settings import get_config +from notifications import settings as notification_settings from notifications.utils import id2slug, slug2id from swapper import load_model @@ -36,7 +38,7 @@ else: class NotificationViewList(ListView): template_name = 'notifications/list.html' context_object_name = 'notifications' - paginate_by = settings.get_config()['PAGINATE_BY'] + paginate_by = notification_settings.get_config()['PAGINATE_BY'] @method_decorator(login_required) def dispatch(self, request, *args, **kwargs): @@ -50,7 +52,7 @@ class AllNotificationsList(NotificationViewList): """ def get_queryset(self): - if settings.get_config()['SOFT_DELETE']: + if notification_settings.get_config()['SOFT_DELETE']: qset = self.request.user.notifications.active() else: qset = self.request.user.notifications.all() @@ -69,8 +71,8 @@ def mark_all_as_read(request): _next = request.GET.get('next') - if _next: - return redirect(_next) + if _next and url_has_allowed_host_and_scheme(_next, settings.ALLOWED_HOSTS): + return redirect(iri_to_uri(_next)) return redirect('notifications:unread') @@ -84,8 +86,8 @@ def mark_as_read(request, slug=None): _next = request.GET.get('next') - if _next: - return redirect(_next) + if _next and url_has_allowed_host_and_scheme(_next, settings.ALLOWED_HOSTS): + return redirect(iri_to_uri(_next)) return redirect('notifications:unread') @@ -100,8 +102,8 @@ def mark_as_unread(request, slug=None): _next = request.GET.get('next') - if _next: - return redirect(_next) + if _next and url_has_allowed_host_and_scheme(_next, settings.ALLOWED_HOSTS): + return redirect(iri_to_uri(_next)) return redirect('notifications:unread') @@ -113,7 +115,7 @@ def delete(request, slug=None): notification = get_object_or_404( Notification, recipient=request.user, id=notification_id) - if settings.get_config()['SOFT_DELETE']: + if notification_settings.get_config()['SOFT_DELETE']: notification.deleted = True notification.save() else: @@ -121,8 +123,8 @@ def delete(request, slug=None): _next = request.GET.get('next') - if _next: - return redirect(_next) + if _next and url_has_allowed_host_and_scheme(_next, settings.ALLOWED_HOSTS): + return redirect(iri_to_uri(_next)) return redirect('notifications:all') @@ -160,7 +162,7 @@ def live_unread_notification_list(request): } return JsonResponse(data) - default_num_to_fetch = get_config()['NUM_TO_FETCH'] + default_num_to_fetch = notification_settings.get_config()['NUM_TO_FETCH'] try: # If they don't specify, make it 5. num_to_fetch = request.GET.get('max', default_num_to_fetch) @@ -208,7 +210,7 @@ def live_all_notification_list(request): } return JsonResponse(data) - default_num_to_fetch = get_config()['NUM_TO_FETCH'] + default_num_to_fetch = notification_settings.get_config()['NUM_TO_FETCH'] try: # If they don't specify, make it 5. num_to_fetch = request.GET.get('max', default_num_to_fetch)