fix(#2737): Safe redirect handling

Unified to 1 function for handling next or default URL, 1 function for confirming if URL is safe. Added tests and improved some typing.

amy/extrequests/views.py

@@ -83,8 +83,9 @@
     update_manual_score,
     upload_trainingrequest_manual_score_csv,
 )
+from workshops.utils.urls import safe_next_or_default_url
 from workshops.utils.usernames import create_username
-from workshops.utils.views import failed_to_delete, redirect_with_next_support
+from workshops.utils.views import failed_to_delete
 
 logger = logging.getLogger("amy")
 
@@ -731,7 +732,9 @@ def trainingrequest_details(request, pk):
             person = form.cleaned_data["person"]
             ok = _match_training_request_to_person(request, training_request=req, person=person, create=create)
             if ok:
-                return redirect_with_next_support(request, "trainingrequest_details", req.pk)
+                next_url = request.GET.get("next", None)
+                default_url = reverse("trainingrequest_details", args=[req.pk])
+                return safe_next_or_default_url(next_url, default_url)
 
     else:  # GET request
         # Provide initial value for form.person

amy/workshops/base_views.py

@@ -2,7 +2,6 @@
 from typing import Optional
 
 from anymail.exceptions import AnymailRequestsAPIError
-from django.conf import settings
 from django.contrib import messages
 from django.contrib.auth.mixins import PermissionRequiredMixin
 from django.contrib.messages.views import SuccessMessageMixin
@@ -11,7 +10,6 @@
 from django.db.models import Model, ProtectedError
 from django.http import Http404, HttpResponse, HttpResponseRedirect
 from django.template.loader import get_template
-from django.utils.http import url_has_allowed_host_and_scheme
 from django.views.generic import (
     CreateView,
     DeleteView,
@@ -26,6 +24,7 @@
 
 from workshops.forms import AdminLookupForm, BootstrapHelper
 from workshops.utils.pagination import Paginator, get_pagination_items
+from workshops.utils.urls import safe_next_or_default_url
 from workshops.utils.views import assign, failed_to_delete
 
 
@@ -253,13 +252,10 @@ def form_valid(self, form):
 
 
 class RedirectSupportMixin:
-    def get_success_url(self):
-        default_url = super().get_success_url()
-        next_url = self.request.GET.get("next", None)
-        if next_url is not None and url_has_allowed_host_and_scheme(next_url, allowed_hosts=settings.ALLOWED_HOSTS):
-            return next_url
-        else:
-            return default_url
+    def get_success_url(self) -> str:
+        next_url = self.request.GET.get("next", None)  # type: ignore
+        default_url = super().get_success_url()  # type: ignore
+        return safe_next_or_default_url(next_url, default_url)
 
 
 class PrepopulationSupportMixin:

amy/workshops/tests/test_utils.py

@@ -28,7 +28,7 @@
 )
 from workshops.utils.pagination import Paginator
 from workshops.utils.reports import reports_link, reports_link_hash
-from workshops.utils.urls import safe_next_or_default_url
+from workshops.utils.urls import safe_next_or_default_url, safe_url
 from workshops.utils.usernames import create_username
 from workshops.utils.views import assign
 
@@ -1261,7 +1261,7 @@ def test_func(**kwargs):
 
 
 class TestSafeNextOrDefaultURL(TestCase):
-    def test_default_url_if_next_empty(self):
+    def test_default_url_if_next_empty(self) -> None:
         # Arrange
         next_url = None
         default_url = "/dashboard/"
@@ -1270,7 +1270,7 @@ def test_default_url_if_next_empty(self):
         # Assert
         self.assertEqual(url, default_url)
 
-    def test_default_url_if_next_not_provided(self):
+    def test_default_url_if_next_not_provided(self) -> None:
         # Arrange
         next_url = ""
         default_url = "/dashboard/"
@@ -1279,7 +1279,7 @@ def test_default_url_if_next_not_provided(self):
         # Assert
         self.assertEqual(url, default_url)
 
-    def test_default_url_if_next_not_safe(self):
+    def test_default_url_if_next_not_safe(self) -> None:
         # Arrange
         next_url = "https://google.com"
         default_url = "/dashboard/"
@@ -1288,11 +1288,37 @@ def test_default_url_if_next_not_safe(self):
         # Assert
         self.assertEqual(url, default_url)
 
-    def test_next_url_if_next_safe(self):
+    def test_next_url_if_next_safe(self) -> None:
         # Arrange
         next_url = "/admin/"
         default_url = "/dashboard/"
         # Act
         url = safe_next_or_default_url(next_url, default_url)
         # Assert
         self.assertEqual(url, next_url)
+
+
+class TestSafeUrl(TestCase):
+    def test_default_url_if_next_not_provided(self) -> None:
+        # Arrange
+        next_url = ""
+        # Act
+        result = safe_url(next_url)
+        # Assert
+        self.assertFalse(result)
+
+    def test_default_url_if_next_not_safe(self) -> None:
+        # Arrange
+        next_url = "https://google.com"
+        # Act
+        result = safe_url(next_url)
+        # Assert
+        self.assertFalse(result)
+
+    def test_next_url_if_next_safe(self) -> None:
+        # Arrange
+        next_url = "/admin/"
+        # Act
+        result = safe_url(next_url)
+        # Assert
+        self.assertTrue(result)

amy/workshops/utils/urls.py

@@ -8,3 +8,7 @@ def safe_next_or_default_url(next_url: Optional[str], default: str) -> str:
     if next_url is not None and url_has_allowed_host_and_scheme(next_url, settings.ALLOWED_HOSTS):
         return next_url
     return default
+
+
+def safe_url(url: str) -> bool:
+    return url_has_allowed_host_and_scheme(url, settings.ALLOWED_HOSTS)

amy/workshops/utils/views.py

@@ -1,25 +1,26 @@
 from collections import defaultdict
-from typing import Optional, Protocol
+from typing import Any, Optional, Protocol
 
-from django.conf import settings
 from django.db import IntegrityError
-from django.http import Http404, HttpRequest, HttpResponseRedirect
-from django.shortcuts import redirect, render
-from django.utils.http import url_has_allowed_host_and_scheme
+from django.db.models import Model
+from django.http import Http404, HttpRequest, HttpResponse
+from django.shortcuts import render
 
 from workshops.models import Person
 
 
 class Assignable(Protocol):
     assigned_to: Optional[Person]
 
-    def save(self): ...
+    def save(self) -> None: ...
 
 
-def failed_to_delete(request, object, protected_objects, back=None):
-    context = {
+def failed_to_delete(
+    request: HttpRequest, object: Model, protected_objects: set[Model], back: str | None = None
+) -> HttpResponse:
+    context: dict[str, Any] = {
         "title": "Failed to delete",
-        "back": back or object.get_absolute_url,
+        "back": back or getattr(object, "get_absolute_url"),
         "object": object,
         "refs": defaultdict(list),
     }
@@ -42,16 +43,3 @@ def assign(obj: Assignable, /, person: Optional[Person]) -> None:
         obj.save()
     except IntegrityError:
         raise Http404(f"Unable to assign {person} to {obj}.")
-
-
-def redirect_with_next_support(request: HttpRequest, *args, **kwargs) -> HttpResponseRedirect:
-    """Works in the same way as `redirect` except when there is GET parameter
-    named "next". In that case, user is redirected to the URL from that
-    parameter. If you have a class-based view, use RedirectSupportMixin that
-    does the same."""
-
-    next_url = request.GET.get("next", None)
-    if next_url is not None and url_has_allowed_host_and_scheme(next_url, allowed_hosts=settings.ALLOWED_HOSTS):
-        return redirect(next_url, permanent=False)
-    else:
-        return redirect(*args, permanent=False, **kwargs)

amy/workshops/views.py

@@ -45,7 +45,6 @@
 from django.shortcuts import get_object_or_404, redirect, render
 from django.urls import reverse, reverse_lazy
 from django.utils.html import escape
-from django.utils.http import url_has_allowed_host_and_scheme
 from github.GithubException import GithubException
 import requests
 from reversion.models import Revision, Version
@@ -155,6 +154,7 @@
     upload_person_task_csv,
     verify_upload_person_task,
 )
+from workshops.utils.urls import safe_next_or_default_url, safe_url
 from workshops.utils.usernames import create_username
 from workshops.utils.views import failed_to_delete
 
@@ -761,7 +761,7 @@ def person_password(request, person_id):
 
 @admin_required
 @permission_required(["workshops.delete_person", "workshops.change_person"], raise_exception=True)
-def persons_merge(request):
+def persons_merge(request: HttpRequest) -> HttpResponse:
     """Display two persons side by side on GET and merge them on POST.
 
     If no persons are supplied via GET params, display person selection
@@ -774,8 +774,9 @@ def persons_merge(request):
             "title": "Merge Persons",
             "form": PersonsSelectionForm(),
         }
-        if "next" in request.GET:
-            return redirect(request.GET.get("next", "/"))
+        next_url = request.GET.get("next")
+        if next_url and safe_url(next_url):
+            return redirect(next_url)
         return render(request, "generic_form.html", context)
 
     elif obj_a_pk == obj_b_pk:
@@ -784,8 +785,9 @@ def persons_merge(request):
             "form": PersonsSelectionForm(),
         }
         messages.warning(request, "You cannot merge the same person with themself.")
-        if "next" in request.GET:
-            return redirect(request.GET.get("next", "/"))
+        next_url = request.GET.get("next")
+        if next_url and safe_url(next_url):
+            return redirect(next_url)
         return render(request, "generic_form.html", context)
 
     obj_a = get_object_or_404(Person, pk=obj_a_pk)
@@ -1888,9 +1890,9 @@ class MockAwardDelete(OnlyForAdminsMixin, PermissionRequiredMixin, AMYDeleteView
     object: Award
 
     def back_address(self) -> Optional[str]:
-        fallback_url = reverse("person_edit", args=[self.get_object().person.id])  # type: ignore
+        fallback_url = reverse("person_edit", args=[self.get_object().person.id])
         referrer = self.request.headers.get("Referer", fallback_url)
-        return referrer if url_has_allowed_host_and_scheme(referrer, settings.ALLOWED_HOSTS) else fallback_url
+        return safe_next_or_default_url(referrer, fallback_url)
 
     def before_delete(self, *args, **kwargs):
         """Save for use in `after_delete` method."""