Skip to content

Commit

Permalink
✨ Optionally validate DigiD mock IDP callback urls
Browse files Browse the repository at this point in the history
  • Loading branch information
swrichards committed Jan 6, 2025
1 parent 7cec7f3 commit 7abfb4e
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 1 deletion.
14 changes: 14 additions & 0 deletions digid_eherkenning/mock/conf.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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(

Check warning on line 57 in digid_eherkenning/mock/conf.py

View check run for this annotation

Codecov / codecov/patch

digid_eherkenning/mock/conf.py#L57

Added line #L57 was not covered by tests
"DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS should be a boolean"
)

return flag
30 changes: 29 additions & 1 deletion digid_eherkenning/mock/idp/views/digid.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import logging
from urllib.parse import urlencode

from django.http import HttpResponseBadRequest
from digid_eherkenning.views.base import get_redirect_url, is_relative_path
from django.http import HttpRequest, HttpResponseBadRequest
from django.urls import reverse
from django.views.generic import FormView, TemplateView
from django.utils.http import url_has_allowed_host_and_scheme

from furl import furl

Expand All @@ -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")
Expand All @@ -34,6 +49,19 @@ def dispatch(self, request, *args, **kwargs):
logger.debug("missing 'cancel' parameter")
return HttpResponseBadRequest("missing 'cancel' parameter")

# The principal use-case of the mock IDP is to redirect from and back to the
# same service (simulating th real-world case where the IDP lives elsewhere).
# 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.
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):
Expand Down
7 changes: 7 additions & 0 deletions digid_eherkenning/views/base.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from urllib.parse import urlparse
from django.utils.http import url_has_allowed_host_and_scheme


Expand All @@ -18,3 +19,9 @@ def get_redirect_url(request, redirect_to, require_https=True):
return redirect_to

return ""


def is_relative_path(uri):
"""Check if URI is a relative path by verifying it has no scheme or netloc"""
parsed = urlparse(uri)
return not (parsed.scheme or parsed.netloc)
34 changes: 34 additions & 0 deletions tests/test_mock_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,40 @@ 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_must_be_safe(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_must_be_safe(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(**OVERRIDE_SETTINGS)
@modify_settings(**MODIFY_SETTINGS)
Expand Down

0 comments on commit 7abfb4e

Please sign in to comment.