Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to default_unauthz_handler to remove use of referrer header. #921

Merged
merged 1 commit into from
Feb 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
45 changes: 29 additions & 16 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
++++++
Expand Down Expand Up @@ -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
-------------
Expand Down
4 changes: 2 additions & 2 deletions docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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``.

Expand Down
20 changes: 5 additions & 15 deletions flask_security/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)


Expand Down
68 changes: 50 additions & 18 deletions tests/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
get_form_input,
get_num_queries,
hash_password,
init_app_with_options,
is_authenticated,
json_authenticate,
logout,
populate_data,
Expand Down Expand Up @@ -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, "[email protected]")
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="[email protected]")
app.security.datastore.activate_user(user)
app.security.datastore.commit()
authenticate(client, "[email protected]")
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, "[email protected]")
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 ("[email protected]", "[email protected]"):
authenticate(clients, user)
response = clients.get("/admin_or_editor")
Expand Down
Loading