Merge pull request #86 from maykinmedia/85-validate-mock-idp-callbacks

✨ Optionally validate DigiD mock IDP callback urls

digid_eherkenning/mock/conf.py

@@ -1,4 +1,5 @@
 from django.conf import settings
+from django.core.exceptions import ImproperlyConfigured
 from django.shortcuts import resolve_url
 from django.urls import reverse
 from django.utils.encoding import force_str
@@ -45,3 +46,16 @@ def get_idp_login_url():
         "digid-mock:login"
     )
     return resolve_url(url)
+
+
+def should_validate_idp_callback_urls() -> bool:
+    """
+    Whether to validate that the `next_url` and `cancel_urls` are safe
+    """
+    flag = getattr(settings, "DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS", settings.DEBUG)
+    if not isinstance(flag, bool):
+        raise ImproperlyConfigured(
+            "DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS should be a boolean"
+        )
+
+    return flag

digid_eherkenning/mock/idp/views/digid.py

@@ -1,21 +1,39 @@
 import logging
 from urllib.parse import urlencode
 
-from django.http import HttpResponseBadRequest
+from django.http import HttpRequest, HttpResponseBadRequest
 from django.urls import reverse
 from django.views.generic import FormView, TemplateView
 
 from furl import furl
 
 from digid_eherkenning.mock import conf
 from digid_eherkenning.mock.idp.forms import PasswordLoginForm
+from digid_eherkenning.views.base import get_redirect_url
 
 logger = logging.getLogger(__name__)
 
 
 class _BaseIDPViewMixin(TemplateView):
     page_title = "DigiD: Inloggen"
 
+    @staticmethod
+    def _is_allowed_callback(request: HttpRequest, url: str):
+        """
+        Determine whether `url` is an allowed callback for the mock IDP.
+
+        The callback URL should be a relative path, or match the host of the mock IDP
+        view. If the IDP is served under HTTPS, then the callback URL must also use
+        HTTPS.
+        """
+        return get_redirect_url(
+            request,
+            url,
+            # Unsafe redirects are not uncommon in development settings, but if the IDP
+            # is hosted behind TLS, the callbacks should also be secure.
+            require_https=request.is_secure(),
+        )
+
     def dispatch(self, request, *args, **kwargs):
         # we pass these variables through the URL instead of dealing with POST and sessions
         self.acs_url = self.request.GET.get("acs")
@@ -34,6 +52,21 @@ def dispatch(self, request, *args, **kwargs):
             logger.debug("missing 'cancel' parameter")
             return HttpResponseBadRequest("missing 'cancel' parameter")
 
+        # The principal use-case is that the mock IDP redirect flow all takes place
+        # within the same service. This should generally only be used in development,
+        # but out of an abundance of caution, and also to please security scanners, we
+        # only allow redirects to the same host that is serving the mock IDP should the
+        # IDP be active in world-facing environments (e.g. in acceptance). This behavior
+        # can be disabled with the DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS setting.
+        if conf.should_validate_idp_callback_urls():
+            if not self._is_allowed_callback(request, self.next_url):
+                return HttpResponseBadRequest("'next_url' parameter must be a safe url")
+
+            if not self._is_allowed_callback(request, self.cancel_url):
+                return HttpResponseBadRequest(
+                    "'cancel_url' parameter must be a safe url"
+                )
+
         return super().dispatch(request, *args, **kwargs)
 
     def get_context_data(self, **kwargs):

tests/test_mock_views.py

@@ -1,11 +1,17 @@
 from urllib.parse import urlencode
 
+from django.conf import settings
 from django.contrib.auth import get_user_model
 from django.test import TestCase, modify_settings, override_settings
 from django.urls import reverse, reverse_lazy
 
 from furl import furl
 
+from digid_eherkenning.mock.conf import (
+    ImproperlyConfigured,
+    should_validate_idp_callback_urls,
+)
+
 
 class DigidMockTestCase(TestCase):
     def assertNoDigidURLS(self, response):
@@ -71,6 +77,80 @@ def test_get_returns_valid_response(self):
         self.assertContains(response, reverse("digid-mock:password"))
         self.assertNoDigidURLS(response)
 
+    @override_settings(DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS=True)
+    def test_cancel_url_cannot_have_different_host(self):
+        url = reverse("digid-mock:login")
+        data = {
+            "acs": reverse("digid:acs"),
+            "next": reverse("test-success"),
+            "cancel": "http://some-other-testserver" + reverse("test-index"),
+        }
+        response = self.client.get(url, data=data)
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.content, b"'cancel_url' parameter must be a safe url")
+
+        with override_settings(DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS=False):
+            response = self.client.get(url, data=data)
+            self.assertEqual(response.status_code, 200)
+
+    @override_settings(DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS=True)
+    def test_next_url_cannot_have_different_host(self):
+        url = reverse("digid-mock:login")
+        data = {
+            "acs": reverse("digid:acs"),
+            "next": "http://some-other-testserver" + reverse("test-success"),
+            "cancel": reverse("test-index"),
+        }
+        response = self.client.get(url, data=data)
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.content, b"'next_url' parameter must be a safe url")
+
+        with override_settings(DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS=False):
+            response = self.client.get(url, data=data)
+            self.assertEqual(response.status_code, 200)
+
+    @override_settings(DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS=True)
+    def test_next_and_cancel_url_can_be_relative(self):
+        url = reverse("digid-mock:login")
+        data = {
+            "acs": reverse("digid:acs"),
+            "next": reverse("test-success"),
+            "cancel": reverse("test-index"),
+        }
+        response = self.client.get(url, data=data)
+        self.assertEqual(response.status_code, 200)
+
+    @override_settings(DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS=True)
+    def test_next_and_cancel_url_must_be_secure_if_idp_is_secure(self):
+        url = reverse("digid-mock:login")
+        data = {
+            "acs": reverse("digid:acs"),
+            "next": "http://testserver" + reverse("test-success"),
+            "cancel": "http://testserver" + reverse("test-index"),
+        }
+        response = self.client.get(url, data=data, secure=True)
+        self.assertEqual(response.status_code, 400)
+
+        response = self.client.get(url, data=data, secure=False)
+        self.assertEqual(response.status_code, 200)
+
+    @override_settings(DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS="True")
+    def test_conf_setting_must_be_a_bool(self):
+        with self.assertRaises(ImproperlyConfigured):
+            should_validate_idp_callback_urls()
+
+    @override_settings()
+    def test_conf_setting_defaults_to_debug_flag(self):
+        del settings.DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS
+
+        with override_settings(DEBUG=True):
+            self.assertTrue(should_validate_idp_callback_urls())
+
+        with override_settings(DEBUG=False):
+            self.assertFalse(should_validate_idp_callback_urls())
+
 
 @override_settings(**OVERRIDE_SETTINGS)
 @modify_settings(**MODIFY_SETTINGS)