From 96d3b79c491134f2c2c027a127be92102b45b661 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Fri, 23 Jul 2021 14:34:21 +0300 Subject: [PATCH] Deprecate server_port from request data dictionary `server_port` is unnecessary, since the HTTP Host header sent by the client already includes any non-standard port. In addition, when the Python application server is sitting behind a reverse proxy/TLS terminator, SERVER_PORT is likely to be wrong anyway (since it would be the server port of the non-reverse-proxied server). See https://github.com/onelogin/python3-saml/issues/273#issuecomment-885566427 --- README.md | 5 +- demo-django/demo/views.py | 1 - demo-flask/index.py | 2 - demo-tornado/views.py | 3 +- demo_pyramid/demo_pyramid/views.py | 5 +- src/onelogin/saml2/utils.py | 49 +++++++------------ .../src/OneLogin/saml2_tests/response_test.py | 3 +- tests/src/OneLogin/saml2_tests/utils_test.py | 7 +-- 8 files changed, 24 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 3bbf2a1f..05b5faf2 100644 --- a/README.md +++ b/README.md @@ -573,7 +573,6 @@ This parameter has the following scheme: req = { "http_host": "", "script_name": "", - "server_port": "", "get_data": "", "post_data": "", @@ -594,7 +593,6 @@ def prepare_from_django_request(request): return { 'http_host': request.META['HTTP_HOST'], 'script_name': request.META['PATH_INFO'], - 'server_port': request.META['SERVER_PORT'], 'get_data': request.GET.copy(), 'post_data': request.POST.copy() } @@ -602,8 +600,7 @@ def prepare_from_django_request(request): def prepare_from_flask_request(request): url_data = urlparse(request.url) return { - 'http_host': request.host, - 'server_port': url_data.port, + 'http_host': request.netloc, 'script_name': request.path, 'get_data': request.args.copy(), 'post_data': request.form.copy() diff --git a/demo-django/demo/views.py b/demo-django/demo/views.py index 418ab0c8..6003b821 100644 --- a/demo-django/demo/views.py +++ b/demo-django/demo/views.py @@ -20,7 +20,6 @@ def prepare_django_request(request): 'https': 'on' if request.is_secure() else 'off', 'http_host': request.META['HTTP_HOST'], 'script_name': request.META['PATH_INFO'], - 'server_port': request.META['SERVER_PORT'], 'get_data': request.GET.copy(), # Uncomment if using ADFS as IdP, https://github.com/onelogin/python-saml/pull/144 # 'lowercase_urlencoding': True, diff --git a/demo-flask/index.py b/demo-flask/index.py index b523cb92..5f43292a 100644 --- a/demo-flask/index.py +++ b/demo-flask/index.py @@ -21,11 +21,9 @@ def init_saml_auth(req): def prepare_flask_request(request): # If server is behind proxys or balancers use the HTTP_X_FORWARDED fields - url_data = urlparse(request.url) return { 'https': 'on' if request.scheme == 'https' else 'off', 'http_host': request.host, - 'server_port': url_data.port, 'script_name': request.path, 'get_data': request.args.copy(), # Uncomment if using ADFS as IdP, https://github.com/onelogin/python-saml/pull/144 diff --git a/demo-tornado/views.py b/demo-tornado/views.py index 70cfea68..e5d062aa 100644 --- a/demo-tornado/views.py +++ b/demo-tornado/views.py @@ -157,9 +157,8 @@ def prepare_tornado_request(request): result = { 'https': 'on' if request == 'https' else 'off', - 'http_host': tornado.httputil.split_host_and_port(request.host)[0], + 'http_host': request.host, 'script_name': request.path, - 'server_port': tornado.httputil.split_host_and_port(request.host)[1], 'get_data': dataDict, 'post_data': dataDict, 'query_string': request.query diff --git a/demo_pyramid/demo_pyramid/views.py b/demo_pyramid/demo_pyramid/views.py index 6dab4edc..96434b8b 100644 --- a/demo_pyramid/demo_pyramid/views.py +++ b/demo_pyramid/demo_pyramid/views.py @@ -15,19 +15,16 @@ def init_saml_auth(req): def prepare_pyramid_request(request): - # Uncomment this portion to set the request.scheme and request.server_port + # Uncomment this portion to set the request.scheme # based on the supplied `X-Forwarded` headers. # Useful for running behind reverse proxies or balancers. # # if 'X-Forwarded-Proto' in request.headers: # request.scheme = request.headers['X-Forwarded-Proto'] - # if 'X-Forwarded-Port' in request.headers: - # request.server_port = int(request.headers['X-Forwarded-Port']) return { 'https': 'on' if request.scheme == 'https' else 'off', 'http_host': request.host, - 'server_port': request.server_port, 'script_name': request.path, 'get_data': request.GET.copy(), # Uncomment if using ADFS as IdP, https://github.com/onelogin/python-saml/pull/144 diff --git a/src/onelogin/saml2/utils.py b/src/onelogin/saml2/utils.py index db88bd34..51ab4e00 100644 --- a/src/onelogin/saml2/utils.py +++ b/src/onelogin/saml2/utils.py @@ -10,6 +10,7 @@ """ import base64 +import warnings from copy import deepcopy import calendar from datetime import datetime @@ -254,27 +255,25 @@ def get_self_url_host(request_data): :rtype: string """ current_host = OneLogin_Saml2_Utils.get_self_host(request_data) - port = '' - if OneLogin_Saml2_Utils.is_https(request_data): - protocol = 'https' - else: - protocol = 'http' - - if 'server_port' in request_data and request_data['server_port'] is not None: - port_number = str(request_data['server_port']) - port = ':' + port_number + protocol = 'https' if OneLogin_Saml2_Utils.is_https(request_data) else 'http' - if protocol == 'http' and port_number == '80': - port = '' - elif protocol == 'https' and port_number == '443': - port = '' + if request_data.get('server_port') is not None: + warnings.warn( + 'The server_port key in request data is deprecated. ' + 'The http_host key should include a port, if required.', + category=DeprecationWarning, + ) + port_suffix = ':%s' % request_data['server_port'] + if not current_host.endswith(port_suffix): + if not ((protocol == 'https' and port_suffix == ':443') or (protocol == 'http' and port_suffix == ':80')): + current_host += port_suffix - return '%s://%s%s' % (protocol, current_host, port) + return '%s://%s' % (protocol, current_host) @staticmethod def get_self_host(request_data): """ - Returns the current host. + Returns the current host (which may include a port number part). :param request_data: The request as a dict :type: dict @@ -283,22 +282,11 @@ def get_self_host(request_data): :rtype: string """ if 'http_host' in request_data: - current_host = request_data['http_host'] + return request_data['http_host'] elif 'server_name' in request_data: - current_host = request_data['server_name'] - else: - raise Exception('No hostname defined') - - if ':' in current_host: - current_host_data = current_host.split(':') - possible_port = current_host_data[-1] - try: - int(possible_port) - current_host = current_host_data[0] - except ValueError: - current_host = ':'.join(current_host_data) - - return current_host + warnings.warn("The server_name key in request data is undocumented & deprecated.", category=DeprecationWarning) + return request_data['server_name'] + raise Exception('No hostname defined') @staticmethod def is_https(request_data): @@ -312,6 +300,7 @@ def is_https(request_data): :rtype: boolean """ is_https = 'https' in request_data and request_data['https'] != 'off' + # TODO: this use of server_port should be removed too is_https = is_https or ('server_port' in request_data and str(request_data['server_port']) == '443') return is_https diff --git a/tests/src/OneLogin/saml2_tests/response_test.py b/tests/src/OneLogin/saml2_tests/response_test.py index a51f8e60..35735ef6 100644 --- a/tests/src/OneLogin/saml2_tests/response_test.py +++ b/tests/src/OneLogin/saml2_tests/response_test.py @@ -1534,8 +1534,7 @@ def testIsInValidEncIssues(self): settings_2 = OneLogin_Saml2_Settings(settings_info_2) request_data = { - 'http_host': 'pytoolkit.com', - 'server_port': 8000, + 'http_host': 'pytoolkit.com:8000', 'script_name': '', 'request_uri': '?acs', } diff --git a/tests/src/OneLogin/saml2_tests/utils_test.py b/tests/src/OneLogin/saml2_tests/utils_test.py index 87112b0c..54d9ae66 100644 --- a/tests/src/OneLogin/saml2_tests/utils_test.py +++ b/tests/src/OneLogin/saml2_tests/utils_test.py @@ -190,7 +190,7 @@ def testGetselfhost(self): request_data = { 'http_host': 'example.com:443' } - self.assertEqual('example.com', OneLogin_Saml2_Utils.get_self_host(request_data)) + self.assertEqual('example.com:443', OneLogin_Saml2_Utils.get_self_host(request_data)) request_data = { 'http_host': 'example.com:ok' @@ -211,11 +211,6 @@ def testisHTTPS(self): } self.assertTrue(OneLogin_Saml2_Utils.is_https(request_data)) - request_data = { - 'server_port': '80' - } - self.assertFalse(OneLogin_Saml2_Utils.is_https(request_data)) - request_data = { 'server_port': '443' }