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 diff --git a/flask_appbuilder/tests/security/test_mvc_security.py b/flask_appbuilder/tests/security/test_mvc_security.py index ec18b8015f..9411e88907 100644 --- a/flask_appbuilder/tests/security/test_mvc_security.py +++ b/flask_appbuilder/tests/security/test_mvc_security.py @@ -109,30 +109,100 @@ 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://localhost/users/add", + next_url="http://localhost/path", follow_redirects=False, ) - assert response.location == "http://localhost/users/add" + assert response.location == "http://localhost/path" - def test_db_login_invalid_next_url(self): + def test_db_login_valid_https_scheme_url(self): """ - Test Security invalid 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="https://www.google.com", + next_url="https://localhost/path", + follow_redirects=False, + ) + assert response.location == "https://localhost/path" + + def test_db_login_invalid_external_next_url(self): + """ + Test Security invalid external next URL + """ + self.browser_logout(self.client) + response = self.browser_login( + self.client, + USERNAME_ADMIN, + PASSWORD_ADMIN, + next_url="https://google.com", + follow_redirects=False, + ) + assert response.location == "http://localhost/" + + 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=u"\u0001" + "sample.com", follow_redirects=False, ) assert response.location == "http://localhost/" 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}") diff --git a/flask_appbuilder/utils/base.py b/flask_appbuilder/utils/base.py index 97a7396cd1..cbf284c364 100644 --- a/flask_appbuilder/utils/base.py +++ b/flask_appbuilder/utils/base.py @@ -1,6 +1,7 @@ import logging from typing import Any, Callable -from urllib.parse import urljoin, urlparse +import unicodedata +from urllib.parse import urlparse from flask import current_app, request from flask_babel import gettext @@ -11,11 +12,24 @@ def is_safe_redirect_url(url: str) -> bool: + 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"] 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 + return (not url_info.netloc or url_info.netloc == host_url.netloc) and ( + not scheme or scheme in valid_schemes )