Improve CSRF

.git-blame-ignore-revs

@@ -1,2 +1,4 @@
 # Black
 721d31e9cb02af22e3ad9d579b7b82123527fafe
+# Black 2024
+af6e7176eb54e7e9de77dd6e2cba03630c13f14e

CHANGES.rst

@@ -32,8 +32,10 @@ Fixes
 - (: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'
+- (:pr:`906`) Remove undocumented and untested looking in session for possible 'next'
   redirect location.
+- (:pr:`xxx`) Improve CSRF documentation and testing. Fix bug where a CSRF failure could
+  return an HTML page even if the request was JSON.
 
 Notes
 ++++++
@@ -101,6 +103,12 @@ Backwards Compatibility Concerns
   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.
+- JSON error response has changed due to issue with WTForms form-level errors. When WTForms
+  introduced form-level errors they added it to the form.errors response using `None` as a key.
+  When serializing it, it would turn into "null". However, if there is more than one error
+  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 `""`.
 
 Version 5.3.3
 -------------

docs/patterns.rst

@@ -212,6 +212,7 @@ Flask-Security strives to support various options for both its endpoints (e.g. `
 and the application endpoints (protected with Flask-Security decorators such as :func:`.auth_required`).
 
 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).
 
 
 CSRF: Single-Page-Applications and AJAX/XHR
@@ -288,6 +289,13 @@ Be aware that if you enable this it will ONLY work if you send the session cooki
 .. note::
     It is IMPORTANT that you initialize/call ``CSRFProtect`` PRIOR to initializing Flask_Security.
 
+.. note::
+    Calling CSRFProtect(app) will setup a @before_request handler to verify CSRF - this occurs BEFORE any Flask-Security decorators
+    or other view/form logic. One side effect is that CSRFProtect, on error, will raise a BadRequest error which returns a small
+    piece of HTML by default - your application will need to add a Flask ErrorHandler to change that. Alternatively, and recommended
+    is to set `WTF_CSRF_CHECK_DEFAULT` to `False` - which will disable the @before_request and let Flask-Security handle CSRF protection
+    including properly returning a JSON response if the caller asks for it.
+
 
 Using a Cookie
 --------------
@@ -387,7 +395,7 @@ CSRF: Pro-Tips
        (or clients must use CSRF/session cookie for logging
        in then once they have an authentication token, no further need for cookie).
 
-    #) If you enable CSRFProtect(app) and you want to support non-form based JSON requests,
+    #) If you enable CSRFProtect(app) and you want to send request data as JSON,
        then you must include the CSRF token in the header (e.g. X-CSRF-Token)
 
     #) You must enable CSRFProtect(app) if you want to accept the CSRF token in the request

flask_security/decorators.py

@@ -5,10 +5,12 @@
     Flask-Security decorators module
 
     :copyright: (c) 2012-2019 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.
 """
 
+from __future__ import annotations
+
 from collections import namedtuple
 import datetime
 from functools import wraps
@@ -39,6 +41,9 @@
     url_for_security,
 )
 
+if t.TYPE_CHECKING:  # pragma: no cover
+    from flask.typing import ResponseValue
+
 # Convenient references
 _csrf = LocalProxy(lambda: current_app.extensions["csrf"])
 
@@ -203,14 +208,14 @@ def _check_http_auth():
     return False
 
 
-def handle_csrf(method: t.Optional[str]) -> None:
+def handle_csrf(method: str, json_response: bool = False) -> ResponseValue | None:
     """Invoke CSRF protection based on authentication method.
 
     Usually this is called as part of a decorator, but if that isn't
     appropriate, endpoint code can call this directly.
 
     If CSRF protection is appropriate, this will call flask_wtf::protect() which
-    will raise a ValidationError on CSRF failure.
+    will raise a CSRFError(BadRequest) on CSRF failure.
 
     This routine does nothing if any of these are true:
 
@@ -220,24 +225,39 @@ def handle_csrf(method: t.Optional[str]) -> None:
 
         #) csrfProtect already checked and accepted the token
 
+    This means in the default config - CSRF is done as part of form validation
+    not here. Only if the application calls CSRFProtect(app) will this method
+    do anything. Furthermore - since this is called PRIOR to form instantiation
+    if the request is JSON - it MUST send the csrf_token as a header.
+
     If the passed in method is not in *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.
 
+    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
     """
     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
+        return None
 
     if config_value("CSRF_PROTECT_MECHANISMS"):
         if method in config_value("CSRF_PROTECT_MECHANISMS"):
-            _csrf.protect()  # type: ignore
+            try:
+                _csrf.protect()  # type: ignore
+            except CSRFError as e:
+                if json_response:
+                    payload = json_error_response(errors=e.description)
+                    return _security._render_json(payload, 400, None, None)
+                raise
         else:
             set_request_attr("fs_ignore_csrf", True)
+    return None
 
 
 def http_auth_required(realm: t.Any) -> DecoratedView:
@@ -255,7 +275,9 @@ def decorator(fn):
         @wraps(fn)
         def wrapper(*args, **kwargs):
             if _check_http_auth():
-                handle_csrf("basic")
+                eresponse = handle_csrf("basic", _security._want_json(request))
+                if eresponse:
+                    return eresponse
                 set_request_attr("fs_authn_via", "basic")
                 return current_app.ensure_sync(fn)(*args, **kwargs)
             r = _security.default_http_auth_realm if callable(realm) else realm
@@ -282,7 +304,9 @@ def auth_token_required(fn: DecoratedView) -> DecoratedView:
     @wraps(fn)
     def decorated(*args, **kwargs):
         if _check_token():
-            handle_csrf("token")
+            eresponse = handle_csrf("token", _security._want_json(request))
+            if eresponse:
+                return eresponse
             set_request_attr("fs_authn_via", "token")
             return current_app.ensure_sync(fn)(*args, **kwargs)
         return _security._unauthn_handler(["token"])
@@ -407,7 +431,9 @@ def decorated_view(
                     # in a session cookie...
                     if not check_and_update_authn_fresh(within, grace, method):
                         return _security._reauthn_handler(within, grace)
-                    handle_csrf(method)
+                    eresponse = handle_csrf(method, _security._want_json(request))
+                    if eresponse:
+                        return eresponse
                     set_request_attr("fs_authn_via", method)
                     return current_app.ensure_sync(fn)(*args, **kwargs)
             return _security._unauthn_handler(ams, headers=h)

flask_security/templates/security/change_password.html

@@ -1,18 +1,20 @@
 {% extends "security/base.html" %}
-{% from "security/_macros.html" import render_field_with_errors, render_field %}
+{% from "security/_macros.html" import render_field_with_errors, render_field, render_field_errors, render_form_errors %}
 
 {% block content %}
   {% include "security/_messages.html" %}
   <h1>{{ _fsdomain('Change password') }}</h1>
   <form action="{{ url_for_security('change_password') }}" method="post" name="change_password_form">
     {{ change_password_form.hidden_tag() }}
+    {{ render_form_errors(change_password_form) }}
     {% if active_password %}
       {{ render_field_with_errors(change_password_form.password) }}
     {% else %}
       <h3>{{ _fsdomain('You do not currently have a password - this will add one.') }}</h3>
     {% endif %}
     {{ render_field_with_errors(change_password_form.new_password) }}
     {{ render_field_with_errors(change_password_form.new_password_confirm) }}
+    {{ render_field_errors(change_password_form.csrf_token) }}
     {{ render_field(change_password_form.submit) }}
   </form>
 {% endblock content %}

flask_security/templates/security/forgot_password.html

@@ -1,12 +1,14 @@
 {% extends "security/base.html" %}
-{% from "security/_macros.html" import render_field_with_errors, render_field %}
+{% from "security/_macros.html" import render_field_with_errors, render_field, render_field_errors, render_form_errors %}
 
 {% block content %}
   {% include "security/_messages.html" %}
   <h1>{{ _fsdomain('Send password reset instructions') }}</h1>
   <form action="{{ url_for_security('forgot_password') }}" method="post" name="forgot_password_form">
     {{ forgot_password_form.hidden_tag() }}
+    {{ render_form_errors(forgot_password_form) }}
     {{ render_field_with_errors(forgot_password_form.email) }}
+    {{ render_field_errors(forgot_password_form.csrf_token) }}
     {{ render_field(forgot_password_form.submit) }}
   </form>
   {% include "security/_menu.html" %}

flask_security/templates/security/reset_password.html

@@ -1,13 +1,15 @@
 {% extends "security/base.html" %}
-{% from "security/_macros.html" import render_field_with_errors, render_field %}
+{% from "security/_macros.html" import render_field_with_errors, render_field, render_field_errors, render_form_errors %}
 
 {% block content %}
   {% include "security/_messages.html" %}
   <h1>{{ _fsdomain('Reset password') }}</h1>
   <form action="{{ url_for_security('reset_password', token=reset_password_token) }}" method="post" name="reset_password_form">
     {{ reset_password_form.hidden_tag() }}
+    {{ render_form_errors(reset_password_form) }}
     {{ render_field_with_errors(reset_password_form.password) }}
     {{ render_field_with_errors(reset_password_form.password_confirm) }}
+    {{ render_field_errors(reset_password_form.csrf_token) }}
     {{ render_field(reset_password_form.submit) }}
   </form>
   {% include "security/_menu.html" %}

flask_security/templates/security/two_factor_setup.html

@@ -17,14 +17,15 @@
 #}
 
 {% extends "security/base.html" %}
-{% from "security/_macros.html" import render_field_with_errors, render_field, render_field_no_label, render_field_errors %}
+{% from "security/_macros.html" import render_field_with_errors, render_field, render_field_no_label, render_field_errors, render_form_errors %}
 
 {% block content %}
   {% include "security/_messages.html" %}
   <h1>{{ _fsdomain("Two-factor authentication adds an extra layer of security to your account") }}</h1>
   <h3>{{ _fsdomain("In addition to your username and password, you'll need to use a code.") }}</h3>
   <form action="{{ url_for_security('two_factor_setup') }}" method="post" name="two_factor_setup_form">
     {{ two_factor_setup_form.hidden_tag() }}
+    {{ render_form_errors(two_factor_setup_form) }}
     <div class="fs-div">{{ _fsdomain("Currently setup two-factor method: %(method)s", method=primary_method) }}</div>
     <hr class="fs-gap">
     {% for subfield in two_factor_setup_form.setup %}
@@ -37,6 +38,7 @@ <h3>{{ _fsdomain("In addition to your username and password, you'll need to use
     </div>
     <div class="fs-gap">
       {{ render_field_errors(two_factor_setup_form.setup) }}
+      {{ render_field_errors(two_factor_setup_form.csrf_token) }}
       {{ render_field(two_factor_setup_form.submit) }}
     </div>
     {% if chosen_method=="authenticator" %}

flask_security/templates/security/wan_signin.html

@@ -3,7 +3,7 @@
 #}
 
 {% extends "security/base.html" %}
-{% from "security/_macros.html" import render_field_with_errors, render_field, render_field_errors, prop_next %}
+{% from "security/_macros.html" import render_field_with_errors, render_field, render_field_errors, render_form_errors, prop_next %}
 
 {% block head_scripts %}
   {{ super() }}
@@ -21,11 +21,13 @@ <h1>{{ _fsdomain("Use Your WebAuthn Security Key as a Second Factor") }}</h1>
   {% if not credential_options %}
     <form action="{{ url_for_security('wan_signin') }}{{ prop_next() }}" method="post" name="wan_signin_form" id="wan-signin-form">
       {{ wan_signin_form.hidden_tag() }}
+      {{ render_form_errors(wan_signin_form) }}
       {% if not is_secondary %}
         {{ render_field_with_errors(wan_signin_form.identity) }}
         {{ render_field_with_errors(wan_signin_form.remember) }}
       {% endif %}
       {{ render_field_errors(wan_signin_form.credential) }}
+      {{ render_field_errors(wan_signin_form.csrf_token) }}
       {{ render_field(wan_signin_form.submit) }}
     </form>
   {% else %}