A better fix for open redirect vulnerability.

In the few cases where user input is used for redirecting (via the 'next' parameter) pull apart the URL and quote the path. This will render any of
the bizarre URLs that are relative according to the spec, but interpreted as absolute by many browsers definitely relative (and harmless).

Remove the previous fix including REDIRECT_VALIDATE_MODE and REDIRECT_VALIDATE_RE configuration variables.
The Werkzeug default of sending relative URLs in Location header was restored as well.

.git-blame-ignore-revs

@@ -0,0 +1,2 @@
+# Black
+721d31e9cb02af22e3ad9d579b7b82123527fafe

CHANGES.rst

@@ -19,19 +19,21 @@ Features
 Fixes
 +++++
 
-- (:issue:`845`) us-signin magic link should use fs_uniquifier (not email)
-- (:issue:`893`) Once again work on open-redirect vulnerability - this time due to newer Werkzeug
-- (:pr:`873`) Update Spanish and Italian translations (gissimo)
-- (:pr:`877`) Make AnonymousUser optional and deprecated
-- (:issue:`875`) user_datastore.create_user has side effects on mutable inputs (NoRePercussions)
+- (:issue:`845`) us-signin magic link should use fs_uniquifier. (not email)
+- (:issue:`893`) Improve open-redirect vulnerability mitigation. (see below)
+- (:pr:`873`) Update Spanish and Italian translations. (gissimo)
+- (:pr:`877`) Make AnonymousUser optional and deprecated.
+- (:issue:`875`) user_datastore.create_user has side effects on mutable inputs. (NoRePercussions)
 - (:pr:`878`) The long deprecated _unauthorized_callback/handler has been removed.
 - (:pr:`881`) No longer rely on Flask-Login.unauthorized callback. See below for implications.
-- (:pr:`855`) Improve translations for two-factor method selection (gissimo)
-- (:pr:`866`) Improve German translations (sr-verde)
+- (:pr:`855`) Improve translations for two-factor method selection. (gissimo)
+- (:pr:`866`) Improve German translations. (sr-verde)
 - (:pr:`889`) Improve method translations for unified signin and two factor. Remove support for Flask-Babelex.
-- (:issue:`884`) Oauth re-used POST_LOGIN_VIEW which caused confusion. See below for implications.
-- (:pr:`900`) Improve (and simplify) Two-Factor setup. See below for backwards compatability issues and new functionality.
-- (:pr:`xxx`) Work with py_webauthn 2.0
+- (:issue:`884`) Oauth re-used POST_LOGIN_VIEW which caused confusion. See below for the new configuration and implications.
+- (:pr:`899`) Improve (and simplify) Two-Factor setup. See below for backwards compatability issues and new functionality.
+- (:pr:`901`) Work with py_webauthn 2.0
+- (:pr:`xxx`) Remove undocumented and untested looking in session for possible 'next'
+  redirect location.
 
 Notes
 ++++++
@@ -86,6 +88,19 @@ Backwards Compatibility Concerns
 - Flask-Security no longer configures anything related to Flask-Login's `fresh_login` logic.
   This shouldn't be used - instead use Flask-Security's :meth:`flask_security.auth_required` decorator.
 - Support for Flask-Babelex has been removed. Please convert to Flask-Babel.
+- Open Redirect mitigation. Release 4.1.0 had a fix for :issue:`486` involving a potential
+  open redirect. This was very low priority since the default configuration of Werkzeug (always
+  convert the Location header to absolute URL) rendered the vulnerability un-exploitable. The solution at that
+  time was to add an optional regex looking for these bizarre URLs that from a HTTP spec perspective
+  are relative, but various browsers would interpret as absolute. In Werkzeug release 2.1 the
+  default was changed so that the Location header was allowed to be a relative URL. This made the
+  open redirect vulnerability much more likely to be exploitable. More recently, additional bizarre
+  URLs were found, as documented in :issue:`893`. More work was done and a patch release 5.3.3
+  was published.  This fix utilized changing the Werkzeug default back to absolute and an updated
+  regex. Comments and thoughts by @gmanfuncky proposed a much better solution and that is in 5.4.
+  This implementation is independent of Werkzeug (and relative Location headers are again the default).
+  The entire regex option has been removed.
+  Instead, any user-supplied path used as a redirect is parsed and quoted.
 
 Version 5.3.3
 -------------

docs/patterns.rst

@@ -114,38 +114,9 @@ The following endpoints accept a ``next`` parameter:
     - .us_signin ("/us-signin")
     - .us_verify ("/us-verify")
 
+Flask-Security always quotes the path portion of a user supplied URL.
+This `link <https://www.cve.org/CVERecord?id=CVE-2021-32618>`_ provides background of why simple parsing of URLs isn't enough.
 
-Flask-Security attempts to verify that redirects are always relative.
-FS uses the standard Python library urlsplit() to parse the URL and verify
-that the ``netloc`` hasn't been altered.
-However, many browsers actually accept URLs that should be considered
-relative and perform various stripping and conversions that can cause them
-to be interpreted as absolute. A trivial example of this is:
-
-.. line-block::
-    /login?next=%20///github.com
-
-This will pass the urlsplit() test that it is relative - but many browsers
-will simply strip off the space and interpret it as an absolute URL!
-
-Prior to Werkzeug 2.1, Werkzeug set the response configuration variable
-``autocorrect_location_header = True`` which forced the response `Location`
-header to always be an absolute path - thus effectively squashing any open
-redirect possibility. However since 2.1 it is now `False`.
-
-Flask Security offers
-2 mitigations for this via the :py:data:`SECURITY_REDIRECT_VALIDATE_MODE` and
-:py:data:`SECURITY_REDIRECT_VALIDATE_RE` configuration variables.
-
-- The first mode - `"absolute"`, which is the default, is to once again set Werkzeug's ``autocorrect_location_header``
-  to ``True``. Please note that this is set JUST for Flask-Security's blueprint - not all requests.
-- With the second mode - `"regex"` - FS uses a regular expression to validate all ``next`` parameters to make sure
-  they will be interpreted as `relative`. Be aware that the default regular
-  expression is based on in-the-field testing and it is quite possible that there
-  are other crafted relative URLs that could escape detection.
-
-:py:data:`SECURITY_REDIRECT_VALIDATE_MODE` actually takes a list - so both
-mechanisms can be specified.
 
 .. _pass_validation_topic:
 

flask_security/core.py

@@ -14,7 +14,6 @@
 from datetime import datetime, timedelta
 from dataclasses import dataclass
 import importlib
-import re
 import typing as t
 import warnings
 
@@ -199,8 +198,6 @@
     "REDIRECT_HOST": None,
     "REDIRECT_BEHAVIOR": None,
     "REDIRECT_ALLOW_SUBDOMAINS": False,
-    "REDIRECT_VALIDATE_MODE": ["absolute"],
-    "REDIRECT_VALIDATE_RE": r"^/{4,}|\\{3,}|[\s\000-\037][/\\]{2,}(?![/\\])|[/\\]([^/\\]|/[^/\\])*[/\\].*",  # noqa: E501
     "FORGOT_PASSWORD_TEMPLATE": "security/forgot_password.html",
     "LOGIN_USER_TEMPLATE": "security/login_user.html",
     "REGISTER_USER_TEMPLATE": "security/register_user.html",
@@ -1272,7 +1269,6 @@ def __init__(
         self._mail_util: MailUtil
         self._phone_util: PhoneUtil
         self._password_util: PasswordUtil
-        self._redirect_validate_re: re.Pattern
         self._totp_factory: "Totp"
         self._username_util: UsernameUtil
         self._mf_recovery_codes_util: MfRecoveryCodesUtil
@@ -1530,32 +1526,10 @@ def init_app(
         if cv("OAUTH_ENABLE", app=app):
             self.oauthglue = OAuthGlue(app, self._oauth)
 
-        redirect_validate_mode = cv("REDIRECT_VALIDATE_MODE", app=app) or []
-        if redirect_validate_mode:
-            # should be "regex" or "absolute" or a list of those
-            if not isinstance(redirect_validate_mode, list):
-                redirect_validate_mode = [redirect_validate_mode]
-            if all([m in ["regex", "absolute"] for m in redirect_validate_mode]):
-                if "regex" in redirect_validate_mode:
-                    rvre = cv("REDIRECT_VALIDATE_RE", app=app)
-                    if rvre:
-                        self._redirect_validate_re = re.compile(rvre)
-                    else:
-                        raise ValueError("Must specify REDIRECT_VALIDATE_RE")
-            else:
-                raise ValueError("Invalid value(s) for REDIRECT_VALIDATE_MODE")
-
-        def autocorrect(r):
-            # By setting this (Werkzeug) avoids any open-redirect issues.
-            r.autocorrect_location_header = True
-            return r
-
         # register our blueprint/endpoints
         bp = None
         if self.register_blueprint:
             bp = create_blueprint(app, self, __name__)
-            if "absolute" in redirect_validate_mode:
-                bp.after_request(autocorrect)
             self.two_factor_plugins.create_blueprint(app, bp, self)
             if self.oauthglue:
                 self.oauthglue._create_blueprint(app, bp)

flask_security/utils.py

@@ -16,7 +16,7 @@
 import hmac
 import time
 import typing as t
-from urllib.parse import parse_qsl, urlsplit, urlunsplit, urlencode
+from urllib.parse import parse_qsl, quote, urlsplit, urlunsplit, urlencode
 import urllib.request
 import urllib.error
 import warnings
@@ -587,14 +587,20 @@ def validate_redirect_url(url: str) -> bool:
             return True
         else:
             return False
-    if config_value("REDIRECT_VALIDATE_MODE") == "regex":
-        matcher = _security._redirect_validate_re.match(url)
-        return matcher is None
     return True
 
 
 def get_post_action_redirect(config_key: str) -> str:
-    return propagate_next(find_redirect(config_key), request.form)
+    """
+    There is a security angle here - the result of this method is
+    sent to Flask::redirect() - and we need to be sure that it can't be
+    interpreted as a user-input external URL - that would mean we would
+    have an 'open-redirect' vulnerability.
+    """
+    rurl = propagate_next(find_redirect(config_key), request.form)
+    (scheme, netloc, path, query, fragment) = urlsplit(rurl)
+    safe_url = urlunsplit((scheme, netloc, quote(path), query, fragment))
+    return safe_url
 
 
 def get_post_login_redirect() -> str:
@@ -614,19 +620,15 @@ def get_post_verify_redirect() -> str:
 
 
 def find_redirect(key: str) -> str:
-    """Returns the URL to redirect to after a user logs in successfully.
+    """Returns the URL to redirect to.
 
-    :param key: The session or application configuration key to search for
+    :param key: The  application configuration key to search for
     """
-    session_value = session.pop(key.lower(), None)
-    session_url = None
-    if session_value:
-        session_url = get_url(session_value)
     app_value = current_app.config[key.upper()]
     app_url = None
     if app_value:
         app_url = get_url(app_value)
-    rv = session_url or app_url or str(current_app.config.get("APPLICATION_ROOT", "/"))
+    rv = app_url or str(current_app.config.get("APPLICATION_ROOT", "/"))
     return rv
 
 

tests/test_common.py

@@ -4,7 +4,7 @@
 
     Test common functionality
 
-    :copyright: (c) 2019-2023 by J. Christopher Wagner (jwag).
+    :copyright: (c) 2019-2024 by J. Christopher Wagner (jwag).
     :license: MIT, see LICENSE for more details.
 """
 
@@ -80,6 +80,15 @@ def test_authenticate_with_invalid_next(client, get_message):
     assert get_message("INVALID_REDIRECT") in response.data
 
 
+@pytest.mark.settings(flash_messages=False)
+def test_authenticate_with_invalid_next_json(client, get_message):
+    data = dict(email="matt@lp.com", password="password")
+    response = client.post("/login?next=http://google.com", json=data)
+    assert response.json["response"]["errors"][0].encode() == get_message(
+        "INVALID_REDIRECT"
+    )
+
+
 def test_authenticate_with_invalid_malformed_next(client, get_message):
     data = dict(email="matt@lp.com", password="password")
     response = client.post("/login?next=http:///google.com", data=data)

tests/test_misc.py

@@ -5,7 +5,7 @@
     Lots of tests
 
     :copyright: (c) 2012 by Matt Wright.
-    :copyright: (c) 2019-2023 by J. Christopher Wagner (jwag).
+    :copyright: (c) 2019-2024 by J. Christopher Wagner (jwag).
     :license: MIT, see LICENSE for more details.
 """
 
@@ -65,7 +65,6 @@
     send_mail,
     uia_email_mapper,
     uia_phone_mapper,
-    validate_redirect_url,
     verify_hash,
 )
 
@@ -1302,56 +1301,32 @@ def test_post_security_with_application_root_and_views(app, sqlalchemy_datastore
     assert "/post_logout" in response.location
 
 
-def test_invalid_redirect_config(app, sqlalchemy_datastore, get_message):
-    with pytest.raises(ValueError):
-        init_app_with_options(
-            app,
-            sqlalchemy_datastore,
-            **{"SECURITY_REDIRECT_VALIDATE_MODE": ["regex", "bogus"]},
-        )
-
-
-def test_invalid_redirect_re(app, sqlalchemy_datastore, get_message):
-    with pytest.raises(ValueError):
-        init_app_with_options(
-            app,
-            sqlalchemy_datastore,
-            **{
-                "SECURITY_REDIRECT_VALIDATE_MODE": ["regex"],
-                "SECURITY_REDIRECT_VALIDATE_RE": None,
-            },
-        )
-
-
-@pytest.mark.settings(redirect_validate_mode="regex")
-def test_validate_redirect(app, sqlalchemy_datastore):
-    """
-    Test various possible URLs that urlsplit() shows as relative but
-    many browsers will interpret as absolute - and thus have a
-    open-redirect vulnerability. Note this vulnerability only
-    is viable if the application sets autocorrect_location_header = False
-    """
-    init_app_with_options(app, sqlalchemy_datastore)
-    with app.test_request_context("http://localhost:5001/login"):
-        assert not validate_redirect_url("\\\\\\github.com")
-        assert not validate_redirect_url(" //github.com")
-        assert not validate_redirect_url("\t//github.com")
-        assert not validate_redirect_url(r"/\github.com")
-        assert not validate_redirect_url(r"\/github.com")
-        assert not validate_redirect_url("//github.com")  # this is normal urlsplit
-
-
-@pytest.mark.settings(post_login_view="\\\\\\github.com")
-def test_validate_redirect_default(app, client):
+def test_open_redirect(app, client, get_message):
     """
     Test various possible URLs that urlsplit() shows as relative but
     many browsers will interpret as absolute - and thus have a
-    open-redirect vulnerability. This tests the default configuration for
-    Flask-Security, Flask, Werkzeug
+    open-redirect vulnerability.
     """
-    authenticate(client)
-    response = client.get("/login", follow_redirects=False)
-    assert response.location.startswith("http://localhost")
+    test_urls = [
+        ("\\\\\\github.com", "%5C%5C%5Cgithub.com"),
+        (" //github.com", "%20//github.com"),
+        (r"/\github.com", "/%5Cgithub.com"),
+        (r"\/github.com", "%5C/github.com"),
+        ("//github.com", ""),
+        ("\t//github.com", ""),
+    ]
+    for i, o in test_urls:
+        data = dict(email="matt@lp.com", password="password", next=i)
+        response = client.post("/login", data=data, follow_redirects=False)
+        if response.status_code == 302:
+            # this means it passed form validation but should have been quoted
+            assert check_location(app, response.location, o)
+        elif response.status_code == 200:
+            # should have failed form validation
+            assert get_message("INVALID_REDIRECT") in response.data
+        else:
+            raise AssertionError("Bad response code")
+        logout(client)
 
 
 def test_kwargs():

tests/test_utils.py

@@ -27,7 +27,6 @@
     us_security_token_sent,
 )
 from flask_security.utils import hash_data, hash_password
-from flask_security.utils import config_value as cv
 
 from itsdangerous import BadSignature, SignatureExpired, URLSafeTimedSerializer
 from werkzeug.http import parse_cookie
@@ -68,13 +67,9 @@ def is_authenticated(client, get_message, auth_token=None):
 
 
 def check_location(app, location, expected_base):
-    # verify response location. This can be absolute or relative based
-    # on configuration
-    redirect_validate_mode = cv("REDIRECT_VALIDATE_MODE", app=app) or []
-    if "absolute" in redirect_validate_mode:
-        return location == f"http://localhost{expected_base}"
-    else:
-        return location == expected_base
+    # verify response location. Historically this can be absolute or relative based
+    # on configuration. As of 5.4 and Werkzeug 2.1 it is always relative
+    return location == expected_base
 
 
 def verify_token(client_nc, token, status=None):

tests/view_scaffold.py

@@ -206,14 +206,6 @@ def get_locale():
     except ImportError:
         pass
 
-    @app.after_request
-    def allow_absolute_redirect(r):
-        # This is JUST to test odd possible redirects that look relative but are
-        # interpreted by browsers as absolute.
-        # DON'T SET THIS IN YOUR APPLICATION!
-        r.autocorrect_location_header = False
-        return r
-
     @user_registered.connect_via(app)
     def on_user_registered(myapp, user, confirm_token, **extra):
         flash(f"To confirm {user.email} - go to /confirm/{confirm_token}")