From b746ad15a7beba337471df2f445ac7a2b806b2ec Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 09:56:28 -0700 Subject: [PATCH 01/17] ISS 983 Allow for side redirects with new configuration --- CHANGES.rst | 2 ++ docs/configuration.rst | 15 +++++++++++++++ flask_security/core.py | 1 + flask_security/utils.py | 42 ++++++++++++++++++++++++++++++----------- tests/test_basic.py | 36 +++++++++++++++++++++++++++++++++++ 5 files changed, 85 insertions(+), 11 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index a0a5d5d6..9b02360b 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,6 +12,8 @@ Features & Improvements +++++++++++++++++++++++ - (:issue:`956`) Add support for changing registered user's email (:py:data:`SECURITY_CHANGE_EMAIL`). - (:pr:`xxx`) Change default password hash to argon2 (was bcrypt). See below for details. +- (:issue:`983`) Add new configuration for (:py:data:`REDIRECT_MATCH_SUBDOMAINS`) to allow for + more flexible redirect matching. Fixes +++++ diff --git a/docs/configuration.rst b/docs/configuration.rst index d4438bc3..cb7c2276 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -320,6 +320,21 @@ These configuration keys are used globally across all features. .. versionadded:: 4.0.0 +.. py:data:: SECURITY_REDIRECT_MATCH_SUBDOMAINS + + Define which subdomains are allowed to be redirected to. This is a list of strings + that are matched against the subdomain of the redirect target. If the subdomain + matches, the redirect is allowed. If the subdomain is not in the list, the redirect + is not allowed. This is useful when you have multiple subdomains and you want to + restrict the redirect to a specific set of subdomains. + + Examples: ``['.example.com']`` will allow all subdomains of example.com to be redirected to. + ``['auth.example.com']`` will only allow the auth.example.com subdomain to be redirected to. + + Default: ``[]``. + + .. versionadded:: 5.4.X + .. py:data:: SECURITY_CSRF_PROTECT_MECHANISMS Authentication mechanisms that require CSRF protection. diff --git a/flask_security/core.py b/flask_security/core.py index bfd5e402..efa4be30 100644 --- a/flask_security/core.py +++ b/flask_security/core.py @@ -200,6 +200,7 @@ "REDIRECT_HOST": None, "REDIRECT_BEHAVIOR": None, "REDIRECT_ALLOW_SUBDOMAINS": False, + "REDIRECT_MATCH_SUBDOMAINS": [], "FORGOT_PASSWORD_TEMPLATE": "security/forgot_password.html", "LOGIN_USER_TEMPLATE": "security/login_user.html", "REGISTER_USER_TEMPLATE": "security/register_user.html", diff --git a/flask_security/utils.py b/flask_security/utils.py index 2082ad58..13861e3b 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -616,17 +616,37 @@ def validate_redirect_url(url: str) -> bool: url_base = urlsplit(request.host_url) if (url_next.netloc or url_next.scheme) and url_next.netloc != url_base.netloc: base_domain = current_app.config.get("SERVER_NAME") - if ( - config_value("REDIRECT_ALLOW_SUBDOMAINS") - and base_domain - and ( - url_next.netloc == base_domain - or url_next.netloc.endswith(f".{base_domain}") - ) - ): - return True - else: - return False + + if (config_value("REDIRECT_ALLOW_SUBDOMAINS")): + if (config_value("REDIRECT_MATCH_SUBDOMAINS")): + + # This is a list of allowed sub domains, there are two + # possible ways to match the subdomain: + # 1. The subdomain is in the list of allowed subdomains (strict) + # 2. The subdomain starts with a . and the netloc ends with this (loose) + + allowed_subdomains = config_value("REDIRECT_ALLOWED_SUBDOMAINS") + + if (len(allowed_subdomains) > 0): + if (url_next.netloc in allowed_subdomains): + return True + else: + for subdomain in allowed_subdomains: + if (subdomain.startswith(".") and + url_next.netloc.endswith(subdomain)): + return True + return False + + if ( + base_domain + and ( + url_next.netloc == base_domain + or url_next.netloc.endswith(f".{base_domain}") + ) + ): + return True + else: + return False return True diff --git a/tests/test_basic.py b/tests/test_basic.py index a6d6a8c0..058ca165 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -171,6 +171,42 @@ def test_authenticate_with_invalid_subdomain_next(app, client, get_message): assert get_message("INVALID_REDIRECT") in response.data +def test_authenticate_with_subdomain_fuzzy_match_next(app, client, get_message): + app.config["SERVER_NAME"] = "lp.com" + app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ['.lp.com'] + data = dict(email="matt@lp.com", password="password") + response = client.post("/login?next=http://sub.lp.com", data=data) + assert response.status_code == 302 + + +def test_authenticate_with_subdomain_fuzzy_match_next_invalid(app, client, get_message): + app.config["SERVER_NAME"] = "lp.com" + app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ['foo.lp.com'] + data = dict(email="matt@lp.com", password="password") + response = client.post("/login?next=http://sub.lp.net", data=data) + assert get_message("INVALID_REDIRECT") in response.data + + +def test_authenticate_with_subdomain_fuzzy_match_next_strict(app, client, get_message): + app.config["SERVER_NAME"] = "lp.com" + app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ['sub.lp.com'] + data = dict(email="matt@lp.com", password="password") + response = client.post("/login?next=http://sub.lp.com", data=data) + assert response.status_code == 302 + + +def test_authenticate_with_subdomain_fuzzy_match_next_strict_invalid(app, client, get_message): + app.config["SERVER_NAME"] = "lp.com" + app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ['foo.lp.com'] + data = dict(email="matt@lp.com", password="password") + response = client.post("/login?next=http://sub.lp.com", data=data) + assert get_message("INVALID_REDIRECT") in response.data + + def test_authenticate_with_subdomain_next_default_config(app, client, get_message): app.config["SERVER_NAME"] = "lp.com" data = dict(email="matt@lp.com", password="password") From 5ed6afa31986b8749349c5466b988252da97d4a6 Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 10:03:13 -0700 Subject: [PATCH 02/17] Replace mismatched configuration reference. --- CHANGES.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 9b02360b..357b27b2 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,7 +12,7 @@ Features & Improvements +++++++++++++++++++++++ - (:issue:`956`) Add support for changing registered user's email (:py:data:`SECURITY_CHANGE_EMAIL`). - (:pr:`xxx`) Change default password hash to argon2 (was bcrypt). See below for details. -- (:issue:`983`) Add new configuration for (:py:data:`REDIRECT_MATCH_SUBDOMAINS`) to allow for +- (:issue:`983`) Add new configuration for (:py:data:`SECURITY_REDIRECT_MATCH_SUBDOMAINS`) to allow for more flexible redirect matching. Fixes From 878a561696b8af87626957901d719a1ee25c435c Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 11:08:10 -0700 Subject: [PATCH 03/17] Update docs --- docs/configuration.rst | 8 ++++++++ flask_security/utils.py | 24 +++++++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index cb7c2276..647f42aa 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -328,6 +328,14 @@ These configuration keys are used globally across all features. is not allowed. This is useful when you have multiple subdomains and you want to restrict the redirect to a specific set of subdomains. + For security reasons, if this setting is configured then the default behavior of + allowing all subdomains of SERVER_NAME is disabled. This setting assumes that you + wish to have detailed control over your allowed subdomains. If you do not wish this + behavior, then also include an entry that matches your SERVER_NAME variable. I.E. + if SERVER_NAME is 'example.com' then include '.example.com' in the list. + + This setting requires that :py:data:`SECURITY_REDIRECT_ALLOW_SUBDOMAINS` is set to ``True``. + Examples: ``['.example.com']`` will allow all subdomains of example.com to be redirected to. ``['auth.example.com']`` will only allow the auth.example.com subdomain to be redirected to. diff --git a/flask_security/utils.py b/flask_security/utils.py index 13861e3b..212e03f8 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -617,6 +617,8 @@ def validate_redirect_url(url: str) -> bool: if (url_next.netloc or url_next.scheme) and url_next.netloc != url_base.netloc: base_domain = current_app.config.get("SERVER_NAME") + # Are we allowed to redirect to other hosts outside of ourself? + if (config_value("REDIRECT_ALLOW_SUBDOMAINS")): if (config_value("REDIRECT_MATCH_SUBDOMAINS")): @@ -627,15 +629,35 @@ def validate_redirect_url(url: str) -> bool: allowed_subdomains = config_value("REDIRECT_ALLOWED_SUBDOMAINS") + # Safety check - do we have a list of allowed subdomains? + if (len(allowed_subdomains) > 0): + + # First, let's check if we have a direct match to the list. + if (url_next.netloc in allowed_subdomains): return True + + # Now, let's check for subdomains that start with a dot. + else: for subdomain in allowed_subdomains: + + # We found a subdomain that starts with a dot. + # Let's check if the netloc ends with this subdomain. + if (subdomain.startswith(".") and url_next.netloc.endswith(subdomain)): + + # Gotcha! We found a match. + return True - return False + + # Default to False if we didn't find a match. + + return False + + # Fall through to the original check if we don't have a list of allowed subdomains. if ( base_domain From 3319e2ab2aa06192c621538a13f1f8e3dc549fb9 Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 11:09:22 -0700 Subject: [PATCH 04/17] update documentation with emphasis --- docs/configuration.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index 647f42aa..9b4da106 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -330,7 +330,7 @@ These configuration keys are used globally across all features. For security reasons, if this setting is configured then the default behavior of allowing all subdomains of SERVER_NAME is disabled. This setting assumes that you - wish to have detailed control over your allowed subdomains. If you do not wish this + wish to have **explicit** control over your allowed subdomains. If you do not wish this behavior, then also include an entry that matches your SERVER_NAME variable. I.E. if SERVER_NAME is 'example.com' then include '.example.com' in the list. From f26fbb82818ccc830fa7cb17a5a7c8075cbe30a3 Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 11:20:26 -0700 Subject: [PATCH 05/17] Misspelled ALLOWED -> ALLOW --- flask_security/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flask_security/utils.py b/flask_security/utils.py index 212e03f8..d6e3e20b 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -627,7 +627,7 @@ def validate_redirect_url(url: str) -> bool: # 1. The subdomain is in the list of allowed subdomains (strict) # 2. The subdomain starts with a . and the netloc ends with this (loose) - allowed_subdomains = config_value("REDIRECT_ALLOWED_SUBDOMAINS") + allowed_subdomains = config_value("REDIRECT_ALLOW_SUBDOMAINS") # Safety check - do we have a list of allowed subdomains? From 7c6855fd61ee7405399f0b6290617767b16edc5e Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 11:26:03 -0700 Subject: [PATCH 06/17] ALLOW -> MATCH pulling the wrong configuration --- flask_security/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flask_security/utils.py b/flask_security/utils.py index d6e3e20b..10fa8b3d 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -627,7 +627,7 @@ def validate_redirect_url(url: str) -> bool: # 1. The subdomain is in the list of allowed subdomains (strict) # 2. The subdomain starts with a . and the netloc ends with this (loose) - allowed_subdomains = config_value("REDIRECT_ALLOW_SUBDOMAINS") + allowed_subdomains = config_value("REDIRECT_MATCH_SUBDOMAINS") # Safety check - do we have a list of allowed subdomains? From f8f2aa6e9981824da24aa6aa455ea74aa224a7fa Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 11:33:27 -0700 Subject: [PATCH 07/17] Missing a proper return if REDIRECT_ALLOW_SUBDOMAINS was False. --- flask_security/utils.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/flask_security/utils.py b/flask_security/utils.py index 10fa8b3d..a50e4f0c 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -657,10 +657,12 @@ def validate_redirect_url(url: str) -> bool: return False - # Fall through to the original check if we don't have a list of allowed subdomains. + # Fall through to the original check if we don't have a + # list of allowed subdomains. if ( - base_domain + config_value("REDIRECT_ALLOW_SUBDOMAINS") + and base_domain and ( url_next.netloc == base_domain or url_next.netloc.endswith(f".{base_domain}") @@ -669,6 +671,9 @@ def validate_redirect_url(url: str) -> bool: return True else: return False + else: + return False + return True From cabc1916fcd598d186f567f954785bd513e016a8 Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 11:39:16 -0700 Subject: [PATCH 08/17] Address styling issues in the test suite. --- tests/test_basic.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_basic.py b/tests/test_basic.py index 058ca165..014e458a 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -174,7 +174,7 @@ def test_authenticate_with_invalid_subdomain_next(app, client, get_message): def test_authenticate_with_subdomain_fuzzy_match_next(app, client, get_message): app.config["SERVER_NAME"] = "lp.com" app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True - app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ['.lp.com'] + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = [".lp.com"] data = dict(email="matt@lp.com", password="password") response = client.post("/login?next=http://sub.lp.com", data=data) assert response.status_code == 302 @@ -183,7 +183,7 @@ def test_authenticate_with_subdomain_fuzzy_match_next(app, client, get_message): def test_authenticate_with_subdomain_fuzzy_match_next_invalid(app, client, get_message): app.config["SERVER_NAME"] = "lp.com" app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True - app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ['foo.lp.com'] + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["foo.lp.com"] data = dict(email="matt@lp.com", password="password") response = client.post("/login?next=http://sub.lp.net", data=data) assert get_message("INVALID_REDIRECT") in response.data @@ -192,7 +192,7 @@ def test_authenticate_with_subdomain_fuzzy_match_next_invalid(app, client, get_m def test_authenticate_with_subdomain_fuzzy_match_next_strict(app, client, get_message): app.config["SERVER_NAME"] = "lp.com" app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True - app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ['sub.lp.com'] + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["sub.lp.com"] data = dict(email="matt@lp.com", password="password") response = client.post("/login?next=http://sub.lp.com", data=data) assert response.status_code == 302 @@ -201,7 +201,7 @@ def test_authenticate_with_subdomain_fuzzy_match_next_strict(app, client, get_me def test_authenticate_with_subdomain_fuzzy_match_next_strict_invalid(app, client, get_message): app.config["SERVER_NAME"] = "lp.com" app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True - app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ['foo.lp.com'] + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["foo.lp.com"] data = dict(email="matt@lp.com", password="password") response = client.post("/login?next=http://sub.lp.com", data=data) assert get_message("INVALID_REDIRECT") in response.data From 935e94719aabf37f7a1cce905f0ecbf831f6cfad Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 11:59:53 -0700 Subject: [PATCH 09/17] Fix varying style issues. --- docs/configuration.rst | 4 ++-- flask_security/utils.py | 13 +++++++------ tests/test_basic.py | 4 +++- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index 9b4da106..9ce63bea 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -330,8 +330,8 @@ These configuration keys are used globally across all features. For security reasons, if this setting is configured then the default behavior of allowing all subdomains of SERVER_NAME is disabled. This setting assumes that you - wish to have **explicit** control over your allowed subdomains. If you do not wish this - behavior, then also include an entry that matches your SERVER_NAME variable. I.E. + wish to have **explicit** control over your allowed subdomains. If you do not wish this + behavior, then also include an entry that matches your SERVER_NAME variable. I.E. if SERVER_NAME is 'example.com' then include '.example.com' in the list. This setting requires that :py:data:`SECURITY_REDIRECT_ALLOW_SUBDOMAINS` is set to ``True``. diff --git a/flask_security/utils.py b/flask_security/utils.py index a50e4f0c..ffec8a94 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -619,8 +619,8 @@ def validate_redirect_url(url: str) -> bool: # Are we allowed to redirect to other hosts outside of ourself? - if (config_value("REDIRECT_ALLOW_SUBDOMAINS")): - if (config_value("REDIRECT_MATCH_SUBDOMAINS")): + if config_value("REDIRECT_ALLOW_SUBDOMAINS"): + if config_value("REDIRECT_MATCH_SUBDOMAINS"): # This is a list of allowed sub domains, there are two # possible ways to match the subdomain: @@ -631,11 +631,11 @@ def validate_redirect_url(url: str) -> bool: # Safety check - do we have a list of allowed subdomains? - if (len(allowed_subdomains) > 0): + if len(allowed_subdomains) > 0: # First, let's check if we have a direct match to the list. - if (url_next.netloc in allowed_subdomains): + if url_next.netloc in allowed_subdomains: return True # Now, let's check for subdomains that start with a dot. @@ -646,8 +646,9 @@ def validate_redirect_url(url: str) -> bool: # We found a subdomain that starts with a dot. # Let's check if the netloc ends with this subdomain. - if (subdomain.startswith(".") and - url_next.netloc.endswith(subdomain)): + if subdomain.startswith(".") and url_next.netloc.endswith( + subdomain + ): # Gotcha! We found a match. diff --git a/tests/test_basic.py b/tests/test_basic.py index 014e458a..85d417d4 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -198,7 +198,9 @@ def test_authenticate_with_subdomain_fuzzy_match_next_strict(app, client, get_me assert response.status_code == 302 -def test_authenticate_with_subdomain_fuzzy_match_next_strict_invalid(app, client, get_message): +def test_authenticate_with_subdomain_fuzzy_match_next_strict_invalid( + app, client, get_message +): app.config["SERVER_NAME"] = "lp.com" app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["foo.lp.com"] From dba7ae35fc7b786b8b67feaa64e79707c9cf4545 Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 15:22:01 -0700 Subject: [PATCH 10/17] Strict matching only, no fuzzy matching allowed. --- flask_security/utils.py | 52 +++++++++++------------------------------ tests/test_basic.py | 22 ++--------------- 2 files changed, 16 insertions(+), 58 deletions(-) diff --git a/flask_security/utils.py b/flask_security/utils.py index ffec8a94..1d82ffbc 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -620,49 +620,25 @@ def validate_redirect_url(url: str) -> bool: # Are we allowed to redirect to other hosts outside of ourself? if config_value("REDIRECT_ALLOW_SUBDOMAINS"): - if config_value("REDIRECT_MATCH_SUBDOMAINS"): - - # This is a list of allowed sub domains, there are two - # possible ways to match the subdomain: - # 1. The subdomain is in the list of allowed subdomains (strict) - # 2. The subdomain starts with a . and the netloc ends with this (loose) + # Capture any allowed subdomains + allowed_subdomains = [] + if config_value("REDIRECT_MATCH_SUBDOMAINS"): allowed_subdomains = config_value("REDIRECT_MATCH_SUBDOMAINS") - # Safety check - do we have a list of allowed subdomains? - - if len(allowed_subdomains) > 0: - - # First, let's check if we have a direct match to the list. - - if url_next.netloc in allowed_subdomains: - return True - - # Now, let's check for subdomains that start with a dot. - - else: - for subdomain in allowed_subdomains: - - # We found a subdomain that starts with a dot. - # Let's check if the netloc ends with this subdomain. - - if subdomain.startswith(".") and url_next.netloc.endswith( - subdomain - ): - - # Gotcha! We found a match. - - return True - - # Default to False if we didn't find a match. - - return False - - # Fall through to the original check if we don't have a - # list of allowed subdomains. + # If we have a list of allowed subdomains, check if the next + # URL is in the list + if ( + len(allowed_subdomains) > 0 + and url_next.netloc in allowed_subdomains + ): + return True + # If we don't have a list of allowed subdomains, check if the + # next URL is the same as the base domain or a subdomain of the + # base domain. This is the original behavior. if ( - config_value("REDIRECT_ALLOW_SUBDOMAINS") + len(allowed_subdomains) == 0 and base_domain and ( url_next.netloc == base_domain diff --git a/tests/test_basic.py b/tests/test_basic.py index 85d417d4..f9dd69f0 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -171,25 +171,7 @@ def test_authenticate_with_invalid_subdomain_next(app, client, get_message): assert get_message("INVALID_REDIRECT") in response.data -def test_authenticate_with_subdomain_fuzzy_match_next(app, client, get_message): - app.config["SERVER_NAME"] = "lp.com" - app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True - app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = [".lp.com"] - data = dict(email="matt@lp.com", password="password") - response = client.post("/login?next=http://sub.lp.com", data=data) - assert response.status_code == 302 - - -def test_authenticate_with_subdomain_fuzzy_match_next_invalid(app, client, get_message): - app.config["SERVER_NAME"] = "lp.com" - app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True - app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["foo.lp.com"] - data = dict(email="matt@lp.com", password="password") - response = client.post("/login?next=http://sub.lp.net", data=data) - assert get_message("INVALID_REDIRECT") in response.data - - -def test_authenticate_with_subdomain_fuzzy_match_next_strict(app, client, get_message): +def test_authenticate_with_subdomain_match_next_strict(app, client, get_message): app.config["SERVER_NAME"] = "lp.com" app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["sub.lp.com"] @@ -198,7 +180,7 @@ def test_authenticate_with_subdomain_fuzzy_match_next_strict(app, client, get_me assert response.status_code == 302 -def test_authenticate_with_subdomain_fuzzy_match_next_strict_invalid( +def test_authenticate_with_subdomain_match_next_strict_invalid( app, client, get_message ): app.config["SERVER_NAME"] = "lp.com" From a2ee5461ea994122935c974375623425118d6b02 Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 15:28:28 -0700 Subject: [PATCH 11/17] Simplified logic, and all tests pass. --- flask_security/utils.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/flask_security/utils.py b/flask_security/utils.py index 1d82ffbc..333a5fd6 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -628,16 +628,9 @@ def validate_redirect_url(url: str) -> bool: # If we have a list of allowed subdomains, check if the next # URL is in the list - if ( - len(allowed_subdomains) > 0 - and url_next.netloc in allowed_subdomains - ): + if len(allowed_subdomains) > 0 and url_next.netloc in allowed_subdomains: return True - - # If we don't have a list of allowed subdomains, check if the - # next URL is the same as the base domain or a subdomain of the - # base domain. This is the original behavior. - if ( + elif ( len(allowed_subdomains) == 0 and base_domain and ( @@ -645,6 +638,9 @@ def validate_redirect_url(url: str) -> bool: or url_next.netloc.endswith(f".{base_domain}") ) ): + # If we don't have a list of allowed subdomains, check if the + # next URL is the same as the base domain or a subdomain of the + # base domain. This is the original behavior. return True else: return False From e5f3da00773ea4b9a492f99296e35285502d11fa Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 15:41:22 -0700 Subject: [PATCH 12/17] Adjust the documentation to match the new logic. --- docs/configuration.rst | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index 9ce63bea..d643bb44 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -322,22 +322,19 @@ These configuration keys are used globally across all features. .. py:data:: SECURITY_REDIRECT_MATCH_SUBDOMAINS - Define which subdomains are allowed to be redirected to. This is a list of strings - that are matched against the subdomain of the redirect target. If the subdomain - matches, the redirect is allowed. If the subdomain is not in the list, the redirect - is not allowed. This is useful when you have multiple subdomains and you want to - restrict the redirect to a specific set of subdomains. - - For security reasons, if this setting is configured then the default behavior of - allowing all subdomains of SERVER_NAME is disabled. This setting assumes that you - wish to have **explicit** control over your allowed subdomains. If you do not wish this - behavior, then also include an entry that matches your SERVER_NAME variable. I.E. - if SERVER_NAME is 'example.com' then include '.example.com' in the list. + This attribute specifies a list of permitted subdomains that can be the + target of redirections. Only the subdomains included in this list will be + authorized to receive redirects. + + For enhanced security, configuring this setting will override the default + setting that permits all subdomains of SERVER_NAME. This is based on the + assumption that you desire to have precise control over the host names that + are permitted. It is essential that you provide a comprehensive list of all + the subdomains you intend to allow access to. This setting requires that :py:data:`SECURITY_REDIRECT_ALLOW_SUBDOMAINS` is set to ``True``. - Examples: ``['.example.com']`` will allow all subdomains of example.com to be redirected to. - ``['auth.example.com']`` will only allow the auth.example.com subdomain to be redirected to. + Examples: ``['auth.example.com']`` will only allow the auth.example.com subdomain to be redirected to. Default: ``[]``. From 7543ec934abfc222a12ec21f12ec763d374d74c2 Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 15:42:03 -0700 Subject: [PATCH 13/17] Adjusted the Changelog --- CHANGES.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 357b27b2..feaa6c0b 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -13,7 +13,7 @@ Features & Improvements - (:issue:`956`) Add support for changing registered user's email (:py:data:`SECURITY_CHANGE_EMAIL`). - (:pr:`xxx`) Change default password hash to argon2 (was bcrypt). See below for details. - (:issue:`983`) Add new configuration for (:py:data:`SECURITY_REDIRECT_MATCH_SUBDOMAINS`) to allow for - more flexible redirect matching. + stricter redirect matching. Fixes +++++ From 4942c631a5b0a757859604c9fca9d0b5145ca88c Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Thu, 30 May 2024 15:45:05 -0700 Subject: [PATCH 14/17] fix styling issue --- docs/configuration.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index d643bb44..118b4090 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -322,12 +322,12 @@ These configuration keys are used globally across all features. .. py:data:: SECURITY_REDIRECT_MATCH_SUBDOMAINS - This attribute specifies a list of permitted subdomains that can be the + This attribute specifies a list of permitted subdomains that can be the target of redirections. Only the subdomains included in this list will be authorized to receive redirects. - For enhanced security, configuring this setting will override the default - setting that permits all subdomains of SERVER_NAME. This is based on the + For enhanced security, configuring this setting will override the default + setting that permits all subdomains of SERVER_NAME. This is based on the assumption that you desire to have precise control over the host names that are permitted. It is essential that you provide a comprehensive list of all the subdomains you intend to allow access to. From 4966715febfb401116cb82e81bc0a79abc6968ba Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Fri, 31 May 2024 10:40:06 -0700 Subject: [PATCH 15/17] updated tests to demonstrate side-domain desired effect. --- flask_security/utils.py | 24 +++++++++++++++++++++--- tests/test_basic.py | 21 +++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/flask_security/utils.py b/flask_security/utils.py index 333a5fd6..e2fdeadf 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -19,7 +19,7 @@ import hmac import time import typing as t -from urllib.parse import parse_qsl, quote, urlsplit, urlunsplit, urlencode +from urllib.parse import parse_qsl, quote, urlsplit, urlunsplit, urlencode, urlparse import urllib.request import urllib.error import warnings @@ -617,8 +617,12 @@ def validate_redirect_url(url: str) -> bool: if (url_next.netloc or url_next.scheme) and url_next.netloc != url_base.netloc: base_domain = current_app.config.get("SERVER_NAME") - # Are we allowed to redirect to other hosts outside of ourself? + # Some Flask implementations may not have SERVER_NAME set + if not base_domain: + base_domain = url_base.netloc + tld = get_top_level_domain(base_domain) + # Are we allowed to redirect to other hosts outside of ourself? if config_value("REDIRECT_ALLOW_SUBDOMAINS"): # Capture any allowed subdomains allowed_subdomains = [] @@ -628,7 +632,11 @@ def validate_redirect_url(url: str) -> bool: # If we have a list of allowed subdomains, check if the next # URL is in the list - if len(allowed_subdomains) > 0 and url_next.netloc in allowed_subdomains: + if ( + len(allowed_subdomains) > 0 + and url_next.netloc in allowed_subdomains + and url_next.netloc.endswith(f".{tld}") + ): return True elif ( len(allowed_subdomains) == 0 @@ -650,6 +658,16 @@ def validate_redirect_url(url: str) -> bool: return True +def get_top_level_domain(url): + # Extract the hostname from the URL if it's not already a hostname + hostname = urlparse(url).hostname or url + # Split the hostname into parts + hostname_parts = hostname.split(".") + # The TLD is the last part + tld = hostname_parts[-1] if len(hostname_parts) > 1 else "" + return tld + + def get_post_action_redirect( config_key: str, next_loc: FlaskForm | MultiDict | dict | None ) -> str: diff --git a/tests/test_basic.py b/tests/test_basic.py index f9dd69f0..70b1353d 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -198,6 +198,27 @@ def test_authenticate_with_subdomain_next_default_config(app, client, get_messag assert get_message("INVALID_REDIRECT") in response.data +def test_authenticate_with_subdomain_next_invalid_domain(app, client, get_message): + app.config["SERVER_NAME"] = "lp.com" + app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = [ + "github.com", + "something.github.com", + ] + data = dict(email="matt@lp.com", password="password") + response = client.post("/login?next=http://sub.lp.net", data=data) + assert get_message("INVALID_REDIRECT") in response.data + + +def test_authenticate_with_subdomain_next_app_context(app, client, get_message): + app.config["SERVER_NAME"] = "application.lp.com" + app.config["SECURITY_REDIRECT_ALLOW_SUBDOMAINS"] = True + app.config["SECURITY_REDIRECT_MATCH_SUBDOMAINS"] = ["sub.lp.com"] + data = dict(email="matt@lp.com", password="password") + response = client.post("/login?next=http://sub.lp.com", data=data) + assert response.status_code == 302 + + def test_authenticate_case_insensitive_email(app, client): response = authenticate(client, "MATT@lp.com", follow_redirects=True) assert b"Welcome matt@lp.com" in response.data From 3ff640218245bb6bb144cddc934ee6efb796a09c Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Fri, 31 May 2024 11:18:14 -0700 Subject: [PATCH 16/17] revert SERVER_NAME change. --- flask_security/utils.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/flask_security/utils.py b/flask_security/utils.py index e2fdeadf..a1288f27 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -614,14 +614,16 @@ def validate_redirect_url(url: str) -> bool: return False url_next = urlsplit(url) url_base = urlsplit(request.host_url) + if (url_next.netloc or url_next.scheme) and url_next.netloc != url_base.netloc: base_domain = current_app.config.get("SERVER_NAME") # Some Flask implementations may not have SERVER_NAME set if not base_domain: - base_domain = url_base.netloc + base_domain = request.host tld = get_top_level_domain(base_domain) + print(f"tld: {tld}") # Are we allowed to redirect to other hosts outside of ourself? if config_value("REDIRECT_ALLOW_SUBDOMAINS"): # Capture any allowed subdomains @@ -661,10 +663,13 @@ def validate_redirect_url(url: str) -> bool: def get_top_level_domain(url): # Extract the hostname from the URL if it's not already a hostname hostname = urlparse(url).hostname or url + print(f"hostname: {hostname}") # Split the hostname into parts hostname_parts = hostname.split(".") + print(f"hostname_parts: {hostname_parts}") # The TLD is the last part tld = hostname_parts[-1] if len(hostname_parts) > 1 else "" + print(f"tld: {tld}") return tld From d44ba2b38b6291ef5403a7d94a9fe7c23f2377c3 Mon Sep 17 00:00:00 2001 From: Tyler Hardison Date: Fri, 31 May 2024 11:19:43 -0700 Subject: [PATCH 17/17] Add in TLS check,remove all print debug statements --- flask_security/utils.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/flask_security/utils.py b/flask_security/utils.py index a1288f27..5dc659bf 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -623,7 +623,6 @@ def validate_redirect_url(url: str) -> bool: base_domain = request.host tld = get_top_level_domain(base_domain) - print(f"tld: {tld}") # Are we allowed to redirect to other hosts outside of ourself? if config_value("REDIRECT_ALLOW_SUBDOMAINS"): # Capture any allowed subdomains @@ -663,13 +662,10 @@ def validate_redirect_url(url: str) -> bool: def get_top_level_domain(url): # Extract the hostname from the URL if it's not already a hostname hostname = urlparse(url).hostname or url - print(f"hostname: {hostname}") # Split the hostname into parts hostname_parts = hostname.split(".") - print(f"hostname_parts: {hostname_parts}") # The TLD is the last part tld = hostname_parts[-1] if len(hostname_parts) > 1 else "" - print(f"tld: {tld}") return tld