From 9c0edc836499d842f20a8337111262f7460e091a Mon Sep 17 00:00:00 2001
From: Johannes Maron <johannes@maron.family>
Date: Tue, 10 Sep 2024 09:44:54 +0200
Subject: [PATCH] Support redirect on absolute pathes without domain name

---
 emark/views.py      | 58 +++++++++++++++++++++++++++------------------
 tests/test_views.py | 20 ++++++++++++++--
 2 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/emark/views.py b/emark/views.py
index 942ea32..f81bf2e 100644
--- a/emark/views.py
+++ b/emark/views.py
@@ -1,3 +1,4 @@
+import logging
 from urllib.parse import urlparse
 
 from django import http
@@ -8,6 +9,8 @@
 
 from . import models
 
+logger = logging.getLogger(__name__)
+
 # white 1x1 pixel JPEG in bytes:
 #
 # import io
@@ -44,30 +47,39 @@ class EmailClickView(SingleObjectMixin, View):
     def get(self, request, *args, **kwargs):
         self.object = self.get_object()
 
-        redirect_to = request.GET.get("url")
-        # The redirect_to URL is user-provided, so it might be malicious
-        # or malformed. We use Django's URL validation to ensure that it
-        # is safe to redirect to.
+        try:
+            redirect_to = request.GET["url"]
+        except KeyError:
+            logger.warning(
+                "Missing url parameter", extra={"request": request}, exc_info=True
+            )
+            return http.HttpResponseBadRequest("Missing url parameter")
         parsed_url = urlparse(redirect_to)
-        if not parsed_url.netloc:
-            return http.HttpResponseBadRequest("Missing url or malformed parameter")
-
-        domain, _port = split_domain_port(parsed_url.netloc)
-        allowed_hosts = settings.ALLOWED_HOSTS
-        if settings.DEBUG:
-            allowed_hosts = settings.ALLOWED_HOSTS + [
-                ".localhost",
-                "127.0.0.1",
-                "[::1]",
-            ]
-        if any(
-            [
-                not domain,
-                not validate_host(domain, allowed_hosts),
-                request.scheme != parsed_url.scheme,
-            ]
-        ):
-            return http.HttpResponseBadRequest("Missing url or malformed parameter")
+        if parsed_url.netloc:
+            # The redirect_to URL is user-provided, so it might be malicious
+            # or malformed. We use Django's URL validation to ensure that it
+            # is safe to redirect to.
+            domain, _port = split_domain_port(parsed_url.netloc)
+            allowed_hosts = settings.ALLOWED_HOSTS
+            if settings.DEBUG:
+                allowed_hosts = settings.ALLOWED_HOSTS + [
+                    ".localhost",
+                    "127.0.0.1",
+                    "[::1]",
+                ]
+            if any(
+                [
+                    not domain,
+                    not validate_host(domain, allowed_hosts),
+                    request.scheme != parsed_url.scheme,
+                ]
+            ):
+                logger.warning(
+                    "Malformed URL parameter: %s",
+                    redirect_to,
+                    extra={"request": request},
+                )
+                return http.HttpResponseBadRequest("Malformed url parameter")
 
         models.Click.objects.create_for_request(
             request, email=self.object, redirect_url=redirect_to
diff --git a/tests/test_views.py b/tests/test_views.py
index 311f57e..36a60e5 100644
--- a/tests/test_views.py
+++ b/tests/test_views.py
@@ -34,13 +34,15 @@ def test_get__no_email(self, client):
 
 class TestEmailClickView:
     @pytest.mark.django_db
-    def test_get__no_redirect_url(self, client):
+    def test_get__no_redirect_url(self, client, caplog):
         msg = baker.make("emark.Send")
         response = client.get(reverse("emark:email-click", kwargs={"pk": msg.pk}))
         assert response.status_code == 400
+        assert response.content == b"Missing url parameter"
+        assert "Missing url parameter" in caplog.text
 
     @pytest.mark.django_db
-    def test_get__unsafe_redirect_url(self, client, live_server):
+    def test_get__unsafe_redirect_url(self, client, live_server, caplog):
         msg = baker.make("emark.Send")
         redirect_url = "http://external-domain.com/?utm_source=foo"
 
@@ -49,6 +51,8 @@ def test_get__unsafe_redirect_url(self, client, live_server):
         url = f"{url}?{urlencode({'url': redirect_url})}"
         response = client.get(url)
         assert response.status_code == 400
+        assert response.content == b"Malformed url parameter"
+        assert "Malformed URL parameter" in caplog.text
 
     @pytest.mark.django_db
     def test_get__different_schema_redirect_url(self, client, live_server):
@@ -73,6 +77,18 @@ def test_get__subdomain_redirect_url(self, client, live_server, settings):
         response = client.get(url)
         assert response.status_code == 302
 
+    @pytest.mark.django_db
+    def test_get__absolute_path(self, client, live_server, settings):
+        settings.ALLOWED_HOSTS = ["testserver", ".testserver"]
+        msg = baker.make("emark.Send")
+        redirect_url = "/some/path?utm_source=foo"
+
+        url = reverse("emark:email-click", kwargs={"pk": msg.pk})
+
+        url = f"{url}?{urlencode({'url': redirect_url})}"
+        response = client.get(url)
+        assert response.status_code == 302
+
     @pytest.mark.django_db
     def test_get__subdomain_debug(self, client, live_server, settings):
         settings.DEBUG = True