From b16210d2f8706d82d0c57089fe3620b5503e4e7b Mon Sep 17 00:00:00 2001 From: Sidney Richards Date: Mon, 6 Jan 2025 17:43:26 +0100 Subject: [PATCH] :sparkles: Optionally validate DigiD mock IDP callback urls --- digid_eherkenning/mock/conf.py | 14 +++++ digid_eherkenning/mock/idp/views/digid.py | 32 ++++++++++- digid_eherkenning/views/base.py | 8 +++ tests/test_mock_views.py | 66 +++++++++++++++++++++++ 4 files changed, 119 insertions(+), 1 deletion(-) diff --git a/digid_eherkenning/mock/conf.py b/digid_eherkenning/mock/conf.py index 71d68eb..0a69b21 100644 --- a/digid_eherkenning/mock/conf.py +++ b/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", False) + if not isinstance(flag, bool): + raise ImproperlyConfigured( + "DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS should be a boolean" + ) + + return flag diff --git a/digid_eherkenning/mock/idp/views/digid.py b/digid_eherkenning/mock/idp/views/digid.py index dc3701f..6903494 100644 --- a/digid_eherkenning/mock/idp/views/digid.py +++ b/digid_eherkenning/mock/idp/views/digid.py @@ -1,14 +1,16 @@ 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.utils.http import url_has_allowed_host_and_scheme 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, is_relative_path logger = logging.getLogger(__name__) @@ -16,6 +18,19 @@ class _BaseIDPViewMixin(TemplateView): page_title = "DigiD: Inloggen" + @staticmethod + def _is_safe_callback(request: HttpRequest, url: str): + if is_relative_path(url): + return True + + return url_has_allowed_host_and_scheme( + url=url, + allowed_hosts=[ + request.get_host(), + ], + require_https=False, + ) + 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 +49,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_safe_callback(request, self.next_url): + return HttpResponseBadRequest("'next_url' parameter must be a safe url") + + if not self._is_safe_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): diff --git a/digid_eherkenning/views/base.py b/digid_eherkenning/views/base.py index 3021fd4..8d8abe0 100644 --- a/digid_eherkenning/views/base.py +++ b/digid_eherkenning/views/base.py @@ -1,3 +1,5 @@ +from urllib.parse import urlparse + from django.utils.http import url_has_allowed_host_and_scheme @@ -18,3 +20,9 @@ def get_redirect_url(request, redirect_to, require_https=True): return redirect_to return "" + + +def is_relative_path(uri: str) -> bool: + """Check if `uri` is a relative path.""" + parsed = urlparse(uri) + return not (parsed.scheme or parsed.netloc) diff --git a/tests/test_mock_views.py b/tests/test_mock_views.py index 548b235..eadd8b1 100644 --- a/tests/test_mock_views.py +++ b/tests/test_mock_views.py @@ -6,6 +6,11 @@ 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 +76,67 @@ 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": "some-other-http://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_can_be_absolute_if_same_host(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) + 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(**OVERRIDE_SETTINGS) @modify_settings(**MODIFY_SETTINGS)