Skip to content

Commit

Permalink
More CSRF fixes. (#958)
Browse files Browse the repository at this point in the history
For application forms (not Flask-Security forms/views) the CSRF_PROTECT_MECHANISMS didn't stop form level CSRF checks.
This PR changes how we communicate to form-based CSRF whether to proceed or not - used to use a FS-specific request attribute - but that only worked with forms instantiated by Flask-Security. Now, use Flask-WTF request attribute `csrf_valid`.

This also simplified the unauth_csrf() decorator so that the 'fall_through' parameter is no longer needed (and uses have been removed).

Add test for @unauth_csrf decorator raising error. Note that this returns a 400 with 'CSRF token is missing' - unlike the normal form-based CSRF check that returns it as a field error.
  • Loading branch information
jwag956 authored Mar 21, 2024
1 parent 528d9a6 commit 3c9c44d
Show file tree
Hide file tree
Showing 13 changed files with 164 additions and 61 deletions.
2 changes: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ Released xxx
Fixes
+++++
- (:issue:`950`) Regression - some templates no longer getting correct config (thanks pete7863).
- (:issue:`954`) CSRF not properly ignored for application forms using SECURITY_CSRF_PROTECT_MECHANISMS.
- (:pr:`957`) Improve jp translations (e-goto)

Version 5.4.2
-------------
Expand Down
17 changes: 15 additions & 2 deletions docs/patterns.rst
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,19 @@ and the application endpoints (protected with Flask-Security decorators such as
If your application just uses forms that are derived from ``Flask-WTF::Flaskform`` - you are done.
Note that all of Flask-Security's endpoints are form based (regardless of how the request was made).

Behind-The-Scenes
++++++++++++++++++
Depending on configuration, there are 3 places CSRF tokens can be checked:
#) As part of form validation for any form derived from FlaskForm (which all Flask-Security
forms are. An error here is recorded in the ``csrf_token`` field and the calling view
decided whether to return 200 or 400 (all Flask-Security views return 200 with form field errors).
This is the default if no other configuration changes are made.
#) As part of an @before_request handler that Flask-WTF sets up if CSRFprotect() is called. On error
this always returns HTTP 400 and small snippet of HTML. This can be disabled
by setting config["WTF_CSRF_CHECK_DEFAULT"] = True.
#) As part of a Flask-Security decorator (:func:`.unauth_csrf`, :func:`.auth_required`). On
error either a JSON response is returned OR CSRFError exception is raised and 400 is returned with the small snippet of HTML
(the exception and default response is part of Flask-WTF).

CSRF: Single-Page-Applications and AJAX/XHR
++++++++++++++++++++++++++++++++++++++++++++
Expand All @@ -256,7 +269,7 @@ Explicit fetch and send of csrf-token
The current session CSRF token
is returned on every JSON GET request (to a Flask-Security endpoint) as ``response['csrf_token`]``.
For web applications that ARE served via flask, it is even easier to get the csrf-token -
`<https://flask-wtf.readthedocs.io/en/1.0.x/csrf/>`_ gives some useful tips.
`<https://flask-wtf.readthedocs.io/en/1.2.x/csrf/>`_ gives some useful tips.

Armed with the csrf-token, the UI must include that in every mutating operation.
Be careful NOT to include the csrf-token in non-mutating requests (such as GETs).
Expand Down Expand Up @@ -293,7 +306,7 @@ Note that we use the header name ``X-CSRF-Token`` as that is one of the default
headers configured in Flask-WTF (*WTF_CSRF_HEADERS*)

To protect your application's endpoints (that presumably are not using Flask forms),
you need to enable CSRF as described in the FlaskWTF `documentation <https://flask-wtf.readthedocs.io/en/1.1.x/csrf/>`_: ::
you need to enable CSRF as described in the FlaskWTF `documentation <https://flask-wtf.readthedocs.io/en/1.2.x/csrf/>`_: ::

flask_wtf.CSRFProtect(app)

Expand Down
1 change: 1 addition & 0 deletions flask_security/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -1634,6 +1634,7 @@ def _csrf_init(app):

# If they don't want ALL mechanisms protected, then they must
# set WTF_CSRF_CHECK_DEFAULT=False so that our decorators get control.
# And our decorators use csrf.protect()
if cv("CSRF_PROTECT_MECHANISMS", app=app) != AUTHN_MECHANISMS:
if not csrf:
# This isn't good.
Expand Down
47 changes: 26 additions & 21 deletions flask_security/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,14 +221,18 @@ def handle_csrf(method: str, json_response: bool = False) -> ResponseValue | Non
if the request is JSON - it MUST send the csrf_token as a header.
If the passed in method is not in
:py:data:`SECURITY_CSRF_PROTECT_MECHANISMS` then not only
will no CSRF code be run, but a flag in the current context ``fs_ignore_csrf``
will be set so that downstream code knows to ignore any CSRF checks.
:py:data:`SECURITY_CSRF_PROTECT_MECHANISMS` then in addition to
no CSRF code being run, the flask_wtf request global 'csrf_valid' will be set
so that downstream code knows to ignore any CSRF checks.
Returns None if all ok, returns a Response with JSON error if request
wanted JSON - else re-raises the CSRFError exception.
.. versionadded:: 3.3.0
.. versionchanged:: 5.4.3
Use flask_wtf request global 'csrf_valid' instead of our own to handle
application forms that aren't derived from our forms.
"""
if (
not current_app.config.get("WTF_CSRF_ENABLED", False)
Expand All @@ -247,7 +251,7 @@ def handle_csrf(method: str, json_response: bool = False) -> ResponseValue | Non
return _security._render_json(payload, 400, None, None)
raise
return None
set_request_attr("fs_ignore_csrf", True)
set_request_attr("csrf_valid", True) # flask_wtf global
return None


Expand Down Expand Up @@ -419,7 +423,7 @@ def decorated_view(
if mechanism and mechanism():
# successfully authenticated. Basic auth is by definition 'fresh'.
# Note that using token auth is ok - but caller still has to pass
# in a session cookie...
# in a session cookie if freshness checking is required.
if not check_and_update_authn_fresh(within, grace, method):
return _security._reauthn_handler(within, grace)
if eresponse := handle_csrf(method, _security._want_json(request)):
Expand Down Expand Up @@ -447,39 +451,40 @@ def unauth_csrf(
This decorator does nothing if *WTF_CSRF_ENABLED* == **False**.
This decorator will always require CSRF if the caller is authenticated.
This decorator does nothing if the caller is authenticated.
This decorator will suppress CSRF if caller isn't authenticated and has set the
:py:data:`SECURITY_CSRF_IGNORE_UNAUTH_ENDPOINTS` config variable.
:param fall_through: if set to True, then if CSRF fails here - simply keep going.
This is appropriate if underlying view is form based and once the form is
instantiated, the csrf_token will be available.
Note that this can mask some errors such as 'The CSRF session token is missing.'
meaning that the caller didn't send a session cookie and instead the caller
might get a 'The CSRF token is missing.' error.
:py:data:`SECURITY_CSRF_IGNORE_UNAUTH_ENDPOINTS` config variable to **True**.
.. versionadded:: 3.3.0
.. versionchanged:: 5.4.2
The fall_through parameter is now ignored.
Add code to properly handle JSON errors.
"""

def wrapper(fn):
@wraps(fn)
def decorated(*args, **kwargs):
if not current_app.config.get(
"WTF_CSRF_ENABLED", False
) or not current_app.extensions.get("csrf", None):
if (
not current_app.config.get("WTF_CSRF_ENABLED", False)
or not current_app.extensions.get("csrf", None)
or g.get("csrf_valid", False)
):
return current_app.ensure_sync(fn)(*args, **kwargs)

if cv("CSRF_IGNORE_UNAUTH_ENDPOINTS") and not is_user_authenticated(
current_user
):
set_request_attr("fs_ignore_csrf", True)
set_request_attr("csrf_valid", True)
else:
try:
_csrf.protect()
except CSRFError:
if not fall_through:
raise
except CSRFError as e:
if _security._want_json(request):
payload = json_error_response(errors=e.description)
return _security._render_json(payload, 400, None, None)
raise

return current_app.ensure_sync(fn)(*args, **kwargs)

Expand Down
2 changes: 1 addition & 1 deletion flask_security/recovery_codes.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ def mf_recovery_codes() -> ResponseValue:


@anonymous_user_required
@unauth_csrf(fall_through=True)
@unauth_csrf()
def mf_recovery():
"""View for entering a recovery code.
Expand Down
3 changes: 2 additions & 1 deletion flask_security/templates/security/register_user.html
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{% extends "security/base.html" %}
{% from "security/_macros.html" import render_field_with_errors, render_field, render_form_errors %}
{% from "security/_macros.html" import render_field_with_errors, render_field, render_form_errors, render_field_errors %}

{% block content %}
{% include "security/_messages.html" %}
Expand All @@ -13,6 +13,7 @@ <h1>{{ _fsdomain('Register') }}</h1>
{% if register_user_form.password_confirm %}
{{ render_field_with_errors(register_user_form.password_confirm) }}
{% endif %}
{{ render_field_errors(register_user_form.csrf_token) }}
{{ render_field(register_user_form.submit) }}
</form>
{% include "security/_menu.html" %}
Expand Down
2 changes: 1 addition & 1 deletion flask_security/tf_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)


@unauth_csrf(fall_through=True)
@unauth_csrf()
def tf_select() -> ResponseValue:
# Ask user which MFA method they want to use.
# This is used when a user has setup more than one type of 2FA.
Expand Down
4 changes: 2 additions & 2 deletions flask_security/unified_signin.py
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ def _send_code_helper(form, send_magic_link):


@anonymous_user_required
@unauth_csrf(fall_through=True)
@unauth_csrf()
def us_signin_send_code() -> ResponseValue:
"""
Send code view. POST only.
Expand Down Expand Up @@ -533,7 +533,7 @@ def us_verify_send_code() -> ResponseValue:
)


@unauth_csrf(fall_through=True)
@unauth_csrf()
def us_signin() -> ResponseValue:
"""
Unified sign in view.
Expand Down
3 changes: 0 additions & 3 deletions flask_security/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,9 +446,6 @@ def suppress_form_csrf():
If app doesn't want CSRF for unauth endpoints then check if caller is authenticated
or not (many endpoints can be called either way).
"""
if get_request_attr("fs_ignore_csrf"):
# This is the case where CsrfProtect was already called (e.g. @auth_required)
return {"csrf": False}
if config_value("CSRF_IGNORE_UNAUTH_ENDPOINTS") and not is_user_authenticated(
current_user
):
Expand Down
18 changes: 9 additions & 9 deletions flask_security/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def _ctx(endpoint):
return _security._run_ctx_processor(endpoint)


@unauth_csrf(fall_through=True)
@unauth_csrf()
def login() -> "ResponseValue":
"""View function for login view
Expand Down Expand Up @@ -291,7 +291,7 @@ def logout():


@anonymous_user_required
@unauth_csrf(fall_through=True)
@unauth_csrf()
def register() -> "ResponseValue":
"""View function which handles a registration request."""

Expand Down Expand Up @@ -350,7 +350,7 @@ def register() -> "ResponseValue":
)


@unauth_csrf(fall_through=True)
@unauth_csrf()
def send_login():
"""View function that sends login instructions for passwordless login"""
form = build_form_from_request("passwordless_login_form")
Expand Down Expand Up @@ -408,7 +408,7 @@ def token_login(token):
return redirect(get_post_login_redirect())


@unauth_csrf(fall_through=True)
@unauth_csrf()
def send_confirmation():
"""View function which sends confirmation instructions (/confirm)."""
form = t.cast(
Expand Down Expand Up @@ -522,7 +522,7 @@ def confirm_email(token):


@anonymous_user_required
@unauth_csrf(fall_through=True)
@unauth_csrf()
def forgot_password():
"""View function that handles a forgotten password request (/reset)."""
form = t.cast(ForgotPasswordForm, build_form_from_request("forgot_password_form"))
Expand Down Expand Up @@ -570,7 +570,7 @@ def forgot_password():


@anonymous_user_required
@unauth_csrf(fall_through=True)
@unauth_csrf()
def reset_password(token):
"""View function that handles a reset password request (/reset/<token>).
Expand Down Expand Up @@ -725,7 +725,7 @@ def change_password():
)


@unauth_csrf(fall_through=True)
@unauth_csrf()
def two_factor_setup():
"""View function for two-factor setup.
Expand Down Expand Up @@ -890,7 +890,7 @@ def two_factor_setup():
)


@unauth_csrf(fall_through=True)
@unauth_csrf()
def two_factor_token_validation():
"""View function for two-factor token validation
Expand Down Expand Up @@ -998,7 +998,7 @@ def two_factor_token_validation():


@anonymous_user_required
@unauth_csrf(fall_through=True)
@unauth_csrf()
def two_factor_rescue():
"""Function that handles a situation where user can't
enter his two-factor validation code
Expand Down
4 changes: 2 additions & 2 deletions flask_security/webauthn.py
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ def _signin_common(user: User | None, usage: list[str]) -> tuple[t.Any, str]:


@anonymous_user_required
@unauth_csrf(fall_through=True)
@unauth_csrf()
def webauthn_signin() -> ResponseValue:
# This view can be called either as a 'first' authentication or as part of
# 2FA.
Expand Down Expand Up @@ -657,7 +657,7 @@ def webauthn_signin() -> ResponseValue:
)


@unauth_csrf(fall_through=True)
@unauth_csrf()
def webauthn_signin_response(token: str) -> ResponseValue:
is_secondary = all(k in session for k in ["tf_user_id", "tf_state"]) and session[
"tf_state"
Expand Down
Loading

0 comments on commit 3c9c44d

Please sign in to comment.