From 12d6e29b589c7c2cdc60e905063fe2fe4a035e66 Mon Sep 17 00:00:00 2001 From: Chris Wagner Date: Thu, 15 Feb 2024 17:59:18 -0800 Subject: [PATCH] Changes to default_unauthz_handler to remove use of referrer header. Also - if a configured callable returns None or empty - return a 403, just as the default behavior does. Document precisely what happens. closes #904 --- .pre-commit-config.yaml | 2 +- CHANGES.rst | 45 +++++++++++++++--------- docs/configuration.rst | 4 +-- flask_security/decorators.py | 20 +++-------- flask_security/utils.py | 3 +- tests/test_basic.py | 68 ++++++++++++++++++++++++++---------- 6 files changed, 89 insertions(+), 53 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9328c1cd..dd235d53 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -19,7 +19,7 @@ repos: - id: pyupgrade args: [--py38-plus] - repo: https://github.com/psf/black - rev: 24.1.1 + rev: 24.2.0 hooks: - id: black - repo: https://github.com/pycqa/flake8 diff --git a/CHANGES.rst b/CHANGES.rst index 68a56115..b7d1c2a1 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -11,35 +11,42 @@ Released xxx Among other changes, this continues the process of dis-entangling Flask-Security from Flask-Login and may require some application changes due to backwards incompatible changes. -Features -++++++++ +Features & Improvements ++++++++++++++++++++++++ - (:issue:`879`) Work with Flask[async]. view decorators and signals support async handlers. - (:pr:`900`) CI support for python 3.12 +- (:issue:`912`) Improve oauth debugging support. Handle next propagation in a more general way. +- (:pr:`877`) Make AnonymousUser optional and deprecated. +- (:pr:`906`) Remove undocumented and untested looking in session for possible 'next' + redirect location. +- (:pr:`901`) Work with py_webauthn 2.0 +- (:pr:`881`) No longer rely on Flask-Login.unauthorized callback. See below for implications. +- (:pr:`899`) Improve (and simplify) Two-Factor setup. See below for backwards compatability issues and new functionality. +- (:issue:`904`) Changes to default unauthorized handler - remove use of referrer header (see below). + + +Docs and Chores ++++++++++++++++ +- (:pr:`889`) Improve method translations for unified signin and two factor. Remove support for Flask-Babelex. +- (:pr:`911`) Chore - stop setting all config as attributes. init_app(\*\*kwargs) can only + set forms, flags, and utility classes. +- (:pr:`873`) Update Spanish and Italian translations. (gissimo) +- (:pr:`855`) Improve translations for two-factor method selection. (gissimo) +- (:pr:`866`) Improve German translations. (sr-verde) +- (:pr:`911`) Remove deprecation of AUTO_LOGIN_AFTER_CONFIRM - it has a reasonable use case. Fixes +++++ - (: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:`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 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:`906`) Remove undocumented and untested looking in session for possible 'next' - redirect location. - (:pr:`908`) Improve CSRF documentation and testing. Fix bug where a CSRF failure could return an HTML page even if the request was JSON. -- (:pr:`911`) Chore - stop setting all config as attributes. init_app(\*\*kwargs) can only - set forms, flags, and utility classes. -- (:pr:`911`) Remove deprecation of AUTO_LOGIN_AFTER_CONFIRM - it has a reasonable use case. -- (:issue:`912`) Improve oauth debugging support. Handle next propagation in a more general way. +- (:pr:`914`) It was possible that if :data:`SECURITY_EMAIL_VALIDATOR_ARGS` were set that + deliverability would be checked even for login. Notes ++++++ @@ -114,6 +121,12 @@ Backwards Compatibility Concerns the default settings for JSON serialization in Flask attempt to sort the keys - which fails with the `None` key. An issue has been filed with WTForms - and maybe it will be changed. Flask-Security now changes any `None` key to `""`. +- The default unauthorized handler behavior has changed slightly and is now documented. The default + (:data:`SECURITY_UNAUTHORIZED_VIEW` == ``None``) has not changed (a default HTTP 403 response). + The precise behavior when :data:`SECURITY_UNAUTHORIZED_VIEW` was set was never documented. + The important change is that Flask-Security no longer ever looks at the request.referrer header and + will never redirect to it. If an application needs that, it can provide a callable that can return + that or any other header. Version 5.3.3 ------------- diff --git a/docs/configuration.rst b/docs/configuration.rst index 64eb0149..9860b6a0 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -739,8 +739,8 @@ Login/Logout .. py:data:: SECURITY_UNAUTHORIZED_VIEW Specifies the view to redirect to if a user attempts to access a URL/endpoint that they do - not have permission to access. If this value is ``None``, the user is presented with a default - HTTP 403 response. + not have permission to access. This can be a callable (which returns a URL or ``None``) or an endpoint or a URL. + If this value is ``None`` or the configured callable returns ``None`` or empty, the user is presented with a default HTTP 403 response. Default: ``None``. diff --git a/flask_security/decorators.py b/flask_security/decorators.py index d4bccffe..c6db7db1 100644 --- a/flask_security/decorators.py +++ b/flask_security/decorators.py @@ -21,7 +21,6 @@ from flask_principal import Identity, Permission, RoleNeed, identity_changed from flask_wtf.csrf import CSRFError from werkzeug.local import LocalProxy -from werkzeug.routing import BuildError from .proxies import _security, DecoratedView from .signals import user_unauthenticated @@ -135,23 +134,14 @@ def default_unauthz_handler(func_name, params): if _security._want_json(request): payload = json_error_response(errors=unauthz_message) return _security._render_json(payload, 403, None, None) - view = cv("UNAUTHORIZED_VIEW") - if view: + if view := cv("UNAUTHORIZED_VIEW"): if callable(view): - view = view() + if not (redirect_to := view()): + abort(403) else: - try: - view = get_url(view) - except BuildError: - view = None + redirect_to = get_url(view) do_flash(unauthz_message, unauthz_message_type) - redirect_to = "/" - if request.referrer and not request.referrer.split("?")[0].endswith( - request.path - ): - redirect_to = request.referrer - - return redirect(view or redirect_to) + return redirect(redirect_to) abort(403) diff --git a/flask_security/utils.py b/flask_security/utils.py index 3ca6f2c6..50bcb988 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -44,6 +44,7 @@ from itsdangerous import BadSignature, SignatureExpired from werkzeug.local import LocalProxy from werkzeug.datastructures import MultiDict +from werkzeug.routing import BuildError from .quart_compat import best, get_quart_status from .proxies import _security, _datastore, _pwd_context, _hashing_context @@ -483,7 +484,7 @@ def get_url(endpoint_or_url: str, qparams: dict[str, str] | None = None) -> str: """ try: return transform_url(url_for(endpoint_or_url), qparams) - except Exception: + except BuildError: # This is an external URL (no endpoint defined in app) # For (mostly) testing - allow changing/adding the url - for example # add a different host:port for cases where the UI is running diff --git a/tests/test_basic.py b/tests/test_basic.py index 9765d5ac..43f4163d 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -28,6 +28,8 @@ get_form_input, get_num_queries, hash_password, + init_app_with_options, + is_authenticated, json_authenticate, logout, populate_data, @@ -463,34 +465,64 @@ def test_unauthorized_access(client, get_message): assert response.status_code == 403 -@pytest.mark.settings(unauthorized_view=lambda: None) -def test_unauthorized_access_with_referrer(client, get_message): - authenticate(client, "joe@lp.com") - response = client.get("/admin", headers={"referer": "/admin"}) - assert response.location != "/admin" - client.get(response.location) +def test_unauthorized_callable_view(app, sqlalchemy_datastore, get_message): + # Test various options using custom unauthorized view + def unauthz_view(): + from flask import request + + if request.path == "/admin": + return None + elif request.path == "/admin_perm": + return "" + elif request.path == "/admin_and_editor": + return "/profile" + elif request.path == "/simple": + # N.B. security issue - app should verify this is local + return request.referrer + else: + return "not_implemented" + + app.config["SECURITY_UNAUTHORIZED_VIEW"] = unauthz_view + init_app_with_options(app, sqlalchemy_datastore) + client = app.test_client() + # activate tiya + with app.test_request_context("/"): + user = app.security.datastore.find_user(email="tiya@lp.com") + app.security.datastore.activate_user(user) + app.security.datastore.commit() + authenticate(client, "tiya@lp.com") + assert is_authenticated(client, get_message) - response = client.get( - "/admin?a=b", headers={"referer": "http://localhost/admin?x=y"} - ) - assert "/" in response.location - client.get(response.headers["Location"]) + response = client.get("/admin") + assert response.status_code == 403 + response = client.get("/admin_perm") + assert response.status_code == 403 + + response = client.get("/admin_and_editor", follow_redirects=False) + assert check_location(app, response.location, "/profile") + response = client.get(response.location) + assert response.data.count(get_message("UNAUTHORIZED")) == 1 response = client.get( - "/admin", headers={"referer": "/admin"}, follow_redirects=True + "/simple", headers={"referer": "/myhome"}, follow_redirects=False ) - assert response.data.count(get_message("UNAUTHORIZED")) == 1 + assert check_location(app, response.location, "/myhome") + - # When referrer is from another path and unauthorized, - # we expect a temp redirect (302) to the referer - response = client.get("/admin?w=s", headers={"referer": "/profile"}) +def test_unauthorized_url_view(app, sqlalchemy_datastore): + # Test unknown endpoint basically results in redirect to the given string. + app.config["SECURITY_UNAUTHORIZED_VIEW"] = ".myendpoint" + init_app_with_options(app, sqlalchemy_datastore) + client = app.test_client() + authenticate(client, "tiya@lp.com") + response = client.get("/admin") assert response.status_code == 302 - assert "/profile" in response.location + check_location(app, response.location, ".myendpoint") @pytest.mark.settings(unauthorized_view="/unauthz") def test_roles_accepted(clients): - # This specificaly tests that we can pass a URL for unauthorized_view. + # This specifically tests that we can pass a URL for unauthorized_view. for user in ("matt@lp.com", "joe@lp.com"): authenticate(clients, user) response = clients.get("/admin_or_editor")