From 3b8d0d64a283d6337a7dc3f9aa2f7554a5c9cc13 Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Mon, 2 Oct 2023 13:32:29 +0200 Subject: [PATCH 1/4] Add additional RelayState URL validation Also, some additional debug logging. Also fixed an issue which would cause logout requests to redirect to the fallback login redirect url --- djangosaml2/utils.py | 24 +++++++++++++++++++++++- djangosaml2/views.py | 14 ++++++++++++++ docs/source/contents/setup.rst | 18 ++++++++++++++++-- 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/djangosaml2/utils.py b/djangosaml2/utils.py index 1b4e824a..1ab50d7a 100644 --- a/djangosaml2/utils.py +++ b/djangosaml2/utils.py @@ -22,6 +22,7 @@ from django.core.exceptions import ImproperlyConfigured from django.http import HttpResponse, HttpResponseRedirect from django.shortcuts import resolve_url +from django.urls import NoReverseMatch from django.utils.http import url_has_allowed_host_and_scheme from saml2.config import SPConfig @@ -99,6 +100,24 @@ def get_fallback_login_redirect_url(): def validate_referral_url(request, url): + # Ensure the url is even a valid URL; sometimes the given url is a + # RelayState containing PySAML data. + # Some technically-valid urls will be fail this check, so the + # SAML_STRICT_URL_VALIDATION setting can be used to turn off this check. + # This should only happen if there is no slash, host and/or protocol in the + # given URL. A better fix would be to add those to the RelayState. + saml_strict_url_validation = getattr( + settings, + "SAML_STRICT_URL_VALIDATION", + True + ) + try: + if saml_strict_url_validation: + url = resolve_url(url) + except NoReverseMatch: + logger.debug("Could not validate given referral url is a valid URL") + return None + # Ensure the user-originating redirection url is safe. # By setting SAML_ALLOWED_HOSTS in settings.py the user may provide a list of "allowed" # hostnames for post-login redirects, much like one would specify ALLOWED_HOSTS . @@ -109,7 +128,10 @@ def validate_referral_url(request, url): ) if not url_has_allowed_host_and_scheme(url=url, allowed_hosts=saml_allowed_hosts): - return get_fallback_login_redirect_url() + logger.debug("Referral URL not in SAML_ALLOWED_HOSTS or of the origin " + "host.") + return None + return url diff --git a/djangosaml2/views.py b/djangosaml2/views.py index 483b77ca..402eec4d 100644 --- a/djangosaml2/views.py +++ b/djangosaml2/views.py @@ -99,6 +99,7 @@ def _get_next_path(request: HttpRequest) -> Optional[str]: return None next_path = validate_referral_url(request, next_path) + return next_path @@ -572,7 +573,15 @@ def post(self, request, attribute_mapping=None, create_unknown_user=None): custom_redirect_url = self.custom_redirect(user, relay_state, session_info) if custom_redirect_url: return HttpResponseRedirect(custom_redirect_url) + relay_state = validate_referral_url(request, relay_state) + if relay_state is None: + logger.debug( + "RelayState is not a valid URL, redirecting to fallback: %s", + relay_state + ) + return HttpResponseRedirect(get_fallback_login_redirect_url()) + logger.debug("Redirecting to the RelayState: %s", relay_state) return HttpResponseRedirect(relay_state) @@ -825,12 +834,17 @@ def finish_logout(request, response): next_path = _get_next_path(request) if next_path is not None: + logger.debug("Redirecting to the RelayState: %s", next_path) return HttpResponseRedirect(next_path) elif settings.LOGOUT_REDIRECT_URL is not None: fallback_url = resolve_url(settings.LOGOUT_REDIRECT_URL) + logger.debug("No valid RelayState found; Redirecting to " + "LOGOUT_REDIRECT_URL") return HttpResponseRedirect(fallback_url) else: current_site = get_current_site(request) + logger.debug("No valid RelayState or LOGOUT_REDIRECT_URL found, " + "rendering fallback template.") return render( request, "registration/logged_out.html", diff --git a/docs/source/contents/setup.rst b/docs/source/contents/setup.rst index a7fda926..c9831b7e 100644 --- a/docs/source/contents/setup.rst +++ b/docs/source/contents/setup.rst @@ -122,11 +122,11 @@ view to djangosaml2 wb path, like ``/saml2/login/``. Handling Post-Login Redirects ============================= -It is often desireable for the client to maintain the URL state (or at least manage it) so that +It is often desirable for the client to maintain the URL state (or at least manage it) so that the URL once authentication has completed is consistent with the desired application state (such as retaining query parameters, etc.) By default, the HttpRequest objects get_host() method is used to determine the hostname of the server, and redirect URL's are allowed so long as the destination -host matches the output of get_host(). However, in some cases it becomes desireable for additional +host matches the output of get_host(). However, in some cases it becomes desirable for additional hostnames to be used for the post-login redirect. In such cases, the setting:: SAML_ALLOWED_HOSTS = [] @@ -138,6 +138,20 @@ In the absence of a ``?next=parameter``, the ``ACS_DEFAULT_REDIRECT_URL`` or ``L be used (assuming the destination hostname either matches the output of get_host() or is included in the ``SAML_ALLOWED_HOSTS`` setting) +Redirect URL validation +======================= + +Djangosaml2 will validate the redirect URL before redirecting to its value. In +some edge-cases, valid redirect targets will fail to pass this check. This is +limited to URLs that are a single 'word' without slashes. (For example, 'home' +but also 'page-with-dashes'). + +In this situation, the best solution would be to add a slash to the URL. For +example: 'home' could be '/home' or 'home/'. +If this is unfeasible, this strict validation can be turned off by setting +``SAML_STRICT_URL_VALIDATION`` to ``False`` in settings.py. + + Preferred sso binding ===================== From 9bfd3df73853053910bf4e3f2871badb0e25e3e4 Mon Sep 17 00:00:00 2001 From: Ty Mees Date: Mon, 2 Oct 2023 15:26:32 +0200 Subject: [PATCH 2/4] Use more generic False-y check This will prevent redirect loops with the strict validation disabled Co-authored-by: Giuseppe De Marco --- djangosaml2/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/djangosaml2/views.py b/djangosaml2/views.py index 402eec4d..44effaab 100644 --- a/djangosaml2/views.py +++ b/djangosaml2/views.py @@ -575,7 +575,7 @@ def post(self, request, attribute_mapping=None, create_unknown_user=None): return HttpResponseRedirect(custom_redirect_url) relay_state = validate_referral_url(request, relay_state) - if relay_state is None: + if not relay_state: logger.debug( "RelayState is not a valid URL, redirecting to fallback: %s", relay_state From 64f6bcee47bb6dfecf9d50fdc17cbd1652d97fe2 Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Mon, 2 Oct 2023 15:43:53 +0200 Subject: [PATCH 3/4] Document named URL patterns also being resolved --- djangosaml2/utils.py | 1 + docs/source/contents/setup.rst | 2 ++ 2 files changed, 3 insertions(+) diff --git a/djangosaml2/utils.py b/djangosaml2/utils.py index 1ab50d7a..b5e24b79 100644 --- a/djangosaml2/utils.py +++ b/djangosaml2/utils.py @@ -113,6 +113,7 @@ def validate_referral_url(request, url): ) try: if saml_strict_url_validation: + # This will also resolve Django URL pattern names url = resolve_url(url) except NoReverseMatch: logger.debug("Could not validate given referral url is a valid URL") diff --git a/docs/source/contents/setup.rst b/docs/source/contents/setup.rst index c9831b7e..886b4128 100644 --- a/docs/source/contents/setup.rst +++ b/docs/source/contents/setup.rst @@ -151,6 +151,8 @@ example: 'home' could be '/home' or 'home/'. If this is unfeasible, this strict validation can be turned off by setting ``SAML_STRICT_URL_VALIDATION`` to ``False`` in settings.py. +During validation, `Django named URL patterns`_ +will also be resolved. Turning off strict validation will prevent this from happening. Preferred sso binding ===================== From add2fe56b9aa82bfacbbcda97450f2dff6607a2c Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Mon, 2 Oct 2023 15:44:13 +0200 Subject: [PATCH 4/4] Bump version to 1.8.0 --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index ab0d7e0d..db40249c 100644 --- a/setup.py +++ b/setup.py @@ -27,7 +27,7 @@ def read(*rnames): setup( name="djangosaml2", - version="1.7.0", + version="1.8.0", description="pysaml2 integration for Django", long_description=read("README.md"), long_description_content_type="text/markdown",