From b5d4c6d4329eab59bdc89bc787b68fcbcbf589e4 Mon Sep 17 00:00:00 2001 From: geido Date: Wed, 6 Apr 2022 15:39:50 +0000 Subject: [PATCH 1/7] Enhance is_safe_redirect_url --- flask_appbuilder/utils/base.py | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/flask_appbuilder/utils/base.py b/flask_appbuilder/utils/base.py index 97a7396cd1..60769b2441 100644 --- a/flask_appbuilder/utils/base.py +++ b/flask_appbuilder/utils/base.py @@ -1,8 +1,10 @@ import logging +import unicodedata + from typing import Any, Callable -from urllib.parse import urljoin, urlparse +from urllib.parse import urlparse -from flask import current_app, request +from flask import current_app from flask_babel import gettext from flask_babel.speaklater import LazyString @@ -11,12 +13,22 @@ def is_safe_redirect_url(url: str) -> bool: - host_url = urlparse(request.host_url) - redirect_url = urlparse(urljoin(request.host_url, url)) - return ( - redirect_url.scheme in ("http", "https") - and host_url.netloc == redirect_url.netloc - ) + if url.startswith("///"): + return False + try: + url_info = urlparse(url) + except ValueError: + return False + if not url_info.netloc and url_info.scheme: + return False + if unicodedata.category(url[0])[0] == "C": + return False + scheme = url_info.scheme + # Consider URLs without a scheme (e.g. //example.com/p) to be http. + if not url_info.scheme and url_info.netloc: + scheme = "http" + valid_schemes = ["http", "https"] + return not scheme or scheme in valid_schemes def get_safe_redirect(url): From a6d85f762ee0b33ae0466ae7f17448fc5b41d776 Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 7 Apr 2022 13:41:38 +0000 Subject: [PATCH 2/7] Enhance tests --- .../tests/security/test_mvc_security.py | 80 +++++++++++++++++-- flask_appbuilder/utils/base.py | 3 +- 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/flask_appbuilder/tests/security/test_mvc_security.py b/flask_appbuilder/tests/security/test_mvc_security.py index ec18b8015f..9377f5c9e5 100644 --- a/flask_appbuilder/tests/security/test_mvc_security.py +++ b/flask_appbuilder/tests/security/test_mvc_security.py @@ -118,21 +118,91 @@ def test_db_login_valid_full_next_url(self): self.client, USERNAME_ADMIN, PASSWORD_ADMIN, - next_url="http://localhost/users/add", + next_url="http://sampleurl.com/path", follow_redirects=False, ) - assert response.location == "http://localhost/users/add" + assert response.location == "http://sampleurl.com/path" - def test_db_login_invalid_next_url(self): + def test_db_login_valid_http_scheme_url(self): """ - Test Security invalid next URL + Test Security valid http scheme next URL """ self.browser_logout(self.client) response = self.browser_login( self.client, USERNAME_ADMIN, PASSWORD_ADMIN, - next_url="https://www.google.com", + next_url="http://localhost/path", + follow_redirects=False, + ) + assert response.location == "http://localhost/path" + + def test_db_login_valid_https_scheme_url(self): + """ + Test Security valid https scheme next URL + """ + self.browser_logout(self.client) + response = self.browser_login( + self.client, + USERNAME_ADMIN, + PASSWORD_ADMIN, + next_url="https://localhost/path", + follow_redirects=False, + ) + assert response.location == "https://localhost/path" + + def test_db_login_invalid_scheme_next_url(self): + """ + Test Security invalid scheme next URL + """ + self.browser_logout(self.client) + response = self.browser_login( + self.client, + USERNAME_ADMIN, + PASSWORD_ADMIN, + next_url="ftp://sample", + follow_redirects=False, + ) + assert response.location == "http://localhost/" + + def test_db_login_invalid_localhost_file_next_url(self): + """ + Test Security invalid path to localhost file next URL + """ + self.browser_logout(self.client) + response = self.browser_login( + self.client, + USERNAME_ADMIN, + PASSWORD_ADMIN, + next_url="file:///path", + follow_redirects=False, + ) + assert response.location == "http://localhost/" + + def test_db_login_invalid_no_netloc_with_scheme_next_url(self): + """ + Test Security invalid next URL with no netloc but with scheme + """ + self.browser_logout(self.client) + response = self.browser_login( + self.client, + USERNAME_ADMIN, + PASSWORD_ADMIN, + next_url="http:///sample.com ", + follow_redirects=False, + ) + assert response.location == "http://localhost/" + + def test_db_login_invalid_control_characters_next_url(self): + """ + Test Security invalid next URL with control characters + """ + self.browser_logout(self.client) + response = self.browser_login( + self.client, + USERNAME_ADMIN, + PASSWORD_ADMIN, + next_url="^@sample.com ", follow_redirects=False, ) assert response.location == "http://localhost/" diff --git a/flask_appbuilder/utils/base.py b/flask_appbuilder/utils/base.py index 60769b2441..78076fd908 100644 --- a/flask_appbuilder/utils/base.py +++ b/flask_appbuilder/utils/base.py @@ -1,7 +1,6 @@ import logging -import unicodedata - from typing import Any, Callable +import unicodedata from urllib.parse import urlparse from flask import current_app From 5db93b1866ffb9ddc5e85114da7f82bc15a58b9e Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 7 Apr 2022 13:49:35 +0000 Subject: [PATCH 3/7] Update test --- flask_appbuilder/tests/security/test_mvc_security.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flask_appbuilder/tests/security/test_mvc_security.py b/flask_appbuilder/tests/security/test_mvc_security.py index 9377f5c9e5..cd3e0e005c 100644 --- a/flask_appbuilder/tests/security/test_mvc_security.py +++ b/flask_appbuilder/tests/security/test_mvc_security.py @@ -202,7 +202,7 @@ def test_db_login_invalid_control_characters_next_url(self): self.client, USERNAME_ADMIN, PASSWORD_ADMIN, - next_url="^@sample.com ", + next_url=" sample.com ", follow_redirects=False, ) assert response.location == "http://localhost/" From 835a66c8519814dd57c5d2e28a902b726a07bedf Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 7 Apr 2022 14:02:06 +0000 Subject: [PATCH 4/7] Update test_db_login_invalid_control_characters_next_url --- flask_appbuilder/tests/security/test_mvc_security.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flask_appbuilder/tests/security/test_mvc_security.py b/flask_appbuilder/tests/security/test_mvc_security.py index cd3e0e005c..614ef810c1 100644 --- a/flask_appbuilder/tests/security/test_mvc_security.py +++ b/flask_appbuilder/tests/security/test_mvc_security.py @@ -202,7 +202,7 @@ def test_db_login_invalid_control_characters_next_url(self): self.client, USERNAME_ADMIN, PASSWORD_ADMIN, - next_url=" sample.com ", + next_url=u"\u0001" + "sample.com", follow_redirects=False, ) assert response.location == "http://localhost/" From 3514f99a50f735e9aea1aee03e804c9593ef4700 Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 7 Apr 2022 14:15:28 +0000 Subject: [PATCH 5/7] Update contributing doc --- CONTRIBUTING.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 2fd15600fa..db02629eef 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -69,7 +69,7 @@ Using Postgres .. code-block:: bash - $ nosetests -v flask_appbuilder.tests.test_A_fixture + $ nosetests -v flask_appbuilder.tests.A_fixture 4 - Run a single test From d87e485cfd0fd158b3d6907ab69330b65b8880fd Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 7 Apr 2022 14:17:07 +0000 Subject: [PATCH 6/7] Fix test --- flask_appbuilder/tests/test_mvc_oauth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flask_appbuilder/tests/test_mvc_oauth.py b/flask_appbuilder/tests/test_mvc_oauth.py index 4620226839..42a7184707 100644 --- a/flask_appbuilder/tests/test_mvc_oauth.py +++ b/flask_appbuilder/tests/test_mvc_oauth.py @@ -86,7 +86,7 @@ def test_oauth_login_next_check(self): self.appbuilder.sm.oauth_remotes = {"google": OAuthRemoteMock()} - raw_state = {"next": ["http://www.google.com"]} + raw_state = {"next": ["ftp://sample"]} state = jwt.encode(raw_state, self.app.config["SECRET_KEY"], algorithm="HS256") response = client.get(f"/oauth-authorized/google?state={state}") From e86c70fdc17d0058981a42d676612de5612eb480 Mon Sep 17 00:00:00 2001 From: geido Date: Fri, 8 Apr 2022 14:42:05 +0000 Subject: [PATCH 7/7] Force same netloc --- .../tests/security/test_mvc_security.py | 24 +++++++++---------- flask_appbuilder/utils/base.py | 7 ++++-- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/flask_appbuilder/tests/security/test_mvc_security.py b/flask_appbuilder/tests/security/test_mvc_security.py index 614ef810c1..9411e88907 100644 --- a/flask_appbuilder/tests/security/test_mvc_security.py +++ b/flask_appbuilder/tests/security/test_mvc_security.py @@ -109,47 +109,47 @@ def test_db_login_valid_next_url(self): ) assert response.location == "http://localhost/users/list/" - def test_db_login_valid_full_next_url(self): + def test_db_login_valid_http_scheme_url(self): """ - Test Security valid full next URL + Test Security valid http scheme next URL """ self.browser_logout(self.client) response = self.browser_login( self.client, USERNAME_ADMIN, PASSWORD_ADMIN, - next_url="http://sampleurl.com/path", + next_url="http://localhost/path", follow_redirects=False, ) - assert response.location == "http://sampleurl.com/path" + assert response.location == "http://localhost/path" - def test_db_login_valid_http_scheme_url(self): + def test_db_login_valid_https_scheme_url(self): """ - Test Security valid http scheme next URL + Test Security valid https scheme next URL """ self.browser_logout(self.client) response = self.browser_login( self.client, USERNAME_ADMIN, PASSWORD_ADMIN, - next_url="http://localhost/path", + next_url="https://localhost/path", follow_redirects=False, ) - assert response.location == "http://localhost/path" + assert response.location == "https://localhost/path" - def test_db_login_valid_https_scheme_url(self): + def test_db_login_invalid_external_next_url(self): """ - Test Security valid https scheme next URL + Test Security invalid external next URL """ self.browser_logout(self.client) response = self.browser_login( self.client, USERNAME_ADMIN, PASSWORD_ADMIN, - next_url="https://localhost/path", + next_url="https://google.com", follow_redirects=False, ) - assert response.location == "https://localhost/path" + assert response.location == "http://localhost/" def test_db_login_invalid_scheme_next_url(self): """ diff --git a/flask_appbuilder/utils/base.py b/flask_appbuilder/utils/base.py index 78076fd908..cbf284c364 100644 --- a/flask_appbuilder/utils/base.py +++ b/flask_appbuilder/utils/base.py @@ -3,7 +3,7 @@ import unicodedata from urllib.parse import urlparse -from flask import current_app +from flask import current_app, request from flask_babel import gettext from flask_babel.speaklater import LazyString @@ -27,7 +27,10 @@ def is_safe_redirect_url(url: str) -> bool: if not url_info.scheme and url_info.netloc: scheme = "http" valid_schemes = ["http", "https"] - return not scheme or scheme in valid_schemes + host_url = urlparse(request.host_url) + return (not url_info.netloc or url_info.netloc == host_url.netloc) and ( + not scheme or scheme in valid_schemes + ) def get_safe_redirect(url):